Skip to content

#922 [Bachelor-Thesis] Provide better means for recording demo Videos (SEE Movies) - #978

Open
ThePlays001 wants to merge 17 commits into
masterfrom
922-bachelor-thesis-better-demo-videos
Open

#922 [Bachelor-Thesis] Provide better means for recording demo Videos (SEE Movies)#978
ThePlays001 wants to merge 17 commits into
masterfrom
922-bachelor-thesis-better-demo-videos

Conversation

@ThePlays001

Copy link
Copy Markdown
Collaborator

Added the 'Cinemachine'-Extension, including the 'Splines'-Extension as its dependency, which is required for this thesis.
Implemented structural components for ease of creating and managing individual 'Cinemachine-Scenes' and various GameObjects within these Scenes.
Also implemented an example for an automatic Spline-Dolly with the ISplineDolly interface and provided a Control-Camera on Display 8 to view the Timeline of the currently active 'Cinemachine-Scene'.
To restore changes made in Play-Mode, a CinemachineStateRestorer has been implemented to roughly restore these changes.

This will close #922

…tter means for video recording; includes all changes made for bachelor-thesis
@ThePlays001
ThePlays001 requested a review from koschke July 29, 2026 09:21
@ThePlays001 ThePlays001 self-assigned this Jul 29, 2026
@ThePlays001 ThePlays001 added enhancement New feature or request bachelor or master topic Something that could be worked on in a bachelor or master thesis. labels Jul 29, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few bad patterns I found which you should check.

Comment thread Assets/SEE/Cinemachines/CinemachineSceneRoot.cs Outdated
Comment thread Assets/SEE/Cinemachines/CinemachineSceneRoot.cs Outdated
Comment thread Assets/SEE/Cinemachines/CinemachineSceneRoot.cs Outdated
Comment thread Assets/SEE/Cinemachines/Cinemachines.cs Outdated
Comment thread Assets/SEE/Cinemachines/CinemachinesRoot.cs Outdated
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Outdated
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Outdated
Comment thread Assets/SEE/Cinemachines/Utility/Utility.cs Outdated
Comment thread Assets/SEE/UI/PictureInPicture/PictureInPicture.cs Outdated
Comment thread Assets/SEE/UI/PictureInPicture/pipDataSource.cs Outdated
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

Multiple changes introduce compile-time issues and broken Unity asset references (invalid editor API usage and missing GUID targets) that will prevent the new Cinemachine/PIP features from working reliably.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR integrates Unity’s Cinemachine (and Splines) into SEE and adds an initial “Cinemachines” framework (root + per-scene components, prefabs, and UI assets) intended to simplify creating/managing cinematic camera scenes for recording demo videos (Issue #922).

Changes:

  • Added the com.unity.cinemachine package (with com.unity.splines dependency) to the Unity manifest/lockfile.
  • Introduced new Cinemachines components/utilities (root setup, scene creation, dolly speed controller, state restoration scaffolding) plus prefabs/render textures to support them.
  • Added UI Toolkit assets for picture-in-picture and a control camera view, backed by ScriptableObject data sources.
File summaries
File Description
Packages/packages-lock.json Locks Cinemachine + Splines package resolutions.
Packages/manifest.json Adds com.unity.cinemachine dependency.
Assets/SEE/UI/PictureInPicture/pipDataSource.cs.meta Meta for new PIP data source script.
Assets/SEE/UI/PictureInPicture/pipDataSource.cs ScriptableObject data source for PIP RenderTexture binding.
Assets/SEE/UI/PictureInPicture/PictureInPicture.cs.meta Meta for PIP namespace placeholder.
Assets/SEE/UI/PictureInPicture/PictureInPicture.cs Namespace placeholder for PIP feature area.
Assets/SEE/UI/PictureInPicture.meta Folder meta for PIP scripts.
Assets/SEE/SEE.asmdef Adds assembly references for newly introduced features/dependencies.
Assets/SEE/Cinemachines/Utility/Utility.cs.meta Meta for utility namespace placeholder.
Assets/SEE/Cinemachines/Utility/Utility.cs Namespace placeholder for Cinemachines utility area.
Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs.meta Meta for CinemachinesUtility.
Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Adds editor utility + state restorer + scene/prefab helpers.
Assets/SEE/Cinemachines/Utility.meta Folder meta for Cinemachines utility.
Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs.meta Meta for SimpleSpeedController.
Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs Adds a basic spline-sector speed controller for dolly motion.
Assets/SEE/Cinemachines/Dolly/Dolly.cs.meta Meta for dolly namespace placeholder.
Assets/SEE/Cinemachines/Dolly/Dolly.cs Namespace placeholder for dolly implementations.
Assets/SEE/Cinemachines/Dolly.meta Folder meta for dolly scripts.
Assets/SEE/Cinemachines/CinemachinesRoot.cs.meta Meta for CinemachinesRoot component.
Assets/SEE/Cinemachines/CinemachinesRoot.cs Adds root component to generate structure/assets and manage scenes.
Assets/SEE/Cinemachines/CinemachineSceneRoot.cs.meta Meta for CinemachinesScene component.
Assets/SEE/Cinemachines/CinemachineSceneRoot.cs Adds per-scene component to create cameras/splines/signals and open timeline.
Assets/SEE/Cinemachines/Cinemachines.cs.meta Meta for Cinemachines namespace placeholder.
Assets/SEE/Cinemachines/Cinemachines.cs Namespace placeholder for Cinemachines feature area.
Assets/SEE/Cinemachines.meta Folder meta for Cinemachines scripts.
Assets/Resources/UI/UnityDefaultRuntimeTheme.tss.meta Meta for runtime UI theme.
Assets/Resources/UI/UnityDefaultRuntimeTheme.tss Imports Unity default runtime theme for UI Toolkit.
Assets/Resources/UI/PictureInPicture/RenderTextures/rtpip_blank.renderTexture.meta Meta for blank PIP RenderTexture asset.
Assets/Resources/UI/PictureInPicture/RenderTextures/rtpip_blank.renderTexture Adds a blank RenderTexture asset for PIP UI plumbing.
Assets/Resources/UI/PictureInPicture/RenderTextures.meta Folder meta for PIP render textures.
Assets/Resources/UI/PictureInPicture/pipDataSource.asset.meta Meta for PIP data source asset instance.
Assets/Resources/UI/PictureInPicture/pipDataSource.asset Adds PIP data source asset instance.
Assets/Resources/UI/PictureInPicture/pip.asset.meta Meta for PIP PanelSettings asset.
Assets/Resources/UI/PictureInPicture/pip.asset Adds PanelSettings for PIP UI.
Assets/Resources/UI/PictureInPicture/pip-camera.uxml.meta Meta for PIP camera UXML.
Assets/Resources/UI/PictureInPicture/pip-camera.uxml Adds PIP camera UI layout/binding.
Assets/Resources/UI/PictureInPicture.meta Folder meta for PIP UI resources.
Assets/Resources/UI/Cinemachines/ControlCameraView.uxml.meta Meta for control camera view UXML.
Assets/Resources/UI/Cinemachines/ControlCameraView.uxml Adds control camera viewport UI/binding.
Assets/Resources/UI/Cinemachines/ControlCameraDataSource.asset.meta Meta for control camera data source asset.
Assets/Resources/UI/Cinemachines/ControlCameraDataSource.asset Adds control camera data source asset instance.
Assets/Resources/UI/Cinemachines/ControlCamera.asset.meta Meta for control camera PanelSettings.
Assets/Resources/UI/Cinemachines/ControlCamera.asset Adds PanelSettings for control camera UI.
Assets/Resources/UI/Cinemachines.meta Folder meta for Cinemachines UI resources.
Assets/Resources/UI.meta Folder meta for UI resources root.
Assets/Resources/Prefabs/Players/DesktopPlayer.prefab Adds a Cinemachine-related component to the desktop player camera object.
Assets/Resources/Prefabs/Cinemachines/SceneTemplate.prefab.meta Meta for SceneTemplate prefab.
Assets/Resources/Prefabs/Cinemachines/SceneTemplate.prefab Adds template prefab for Cinemachines scenes (cameras/splines/focus/other roots).
Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/ControlCamera.prefab.meta Meta for ControlCamera prefab.
Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/ControlCamera.prefab Adds control camera prefab with UI document/panel settings.
Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab.meta Meta for CinemachinesBrains prefab.
Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab Adds brain prefab(s) for main + PIP outputs.
Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot.meta Folder meta for CinemachinesRoot prefabs.
Assets/Resources/Prefabs/Cinemachines.meta Folder meta for Cinemachines prefabs.
Assets/Cinemachines/Scenes.meta Adds/marks Scenes folder under Assets/Cinemachines.
Assets/Cinemachines/CinemachinesPIPOutput.renderTexture.meta Meta for PIP output RenderTexture asset.
Assets/Cinemachines/CinemachinesPIPOutput.renderTexture Adds PIP output RenderTexture asset.
Assets/Cinemachines/CinemachinesMainOutput.renderTexture.meta Meta for main output RenderTexture asset.
Assets/Cinemachines/CinemachinesMainOutput.renderTexture Adds main output RenderTexture asset.
Assets/Cinemachines.meta Folder meta for Assets/Cinemachines.
Review details

Files not reviewed (18)

  • Assets/Cinemachines.meta: Generated file
  • Assets/Cinemachines/CinemachinesMainOutput.renderTexture.meta: Generated file
  • Assets/Cinemachines/CinemachinesPIPOutput.renderTexture.meta: Generated file
  • Assets/Cinemachines/Scenes.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/ControlCamera.prefab.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/SceneTemplate.prefab.meta: Generated file
  • Assets/Resources/UI.meta: Generated file
  • Assets/Resources/UI/Cinemachines.meta: Generated file
  • Assets/Resources/UI/Cinemachines/ControlCamera.asset.meta: Generated file
  • Assets/Resources/UI/Cinemachines/ControlCameraDataSource.asset.meta: Generated file
  • Assets/Resources/UI/Cinemachines/ControlCameraView.uxml.meta: Generated file
  • Assets/Resources/UI/PictureInPicture.meta: Generated file
  • Assets/Resources/UI/PictureInPicture/RenderTextures.meta: Generated file
  • Assets/Resources/UI/PictureInPicture/RenderTextures/rtpip_blank.renderTexture.meta: Generated file
  • Assets/Resources/UI/PictureInPicture/pip-camera.uxml.meta: Generated file

Comments suppressed due to low confidence (1)

Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab:250

  • This prefab camera targets a RenderTexture GUID (28773...) that does not exist in the PR, so the PIP Cinemachine brain won't render to the intended output texture. Update it to the CinemachinesPIPOutput.renderTexture GUID introduced in this PR.
  m_RenderingPath: -1
  m_TargetTexture: {fileID: 8400000, guid: 28773ac73b82320b48f9fc5e7787ec21, type: 2}
  m_TargetDisplay: 0
  • Files reviewed: 42/60 changed files
  • Comments generated: 11
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread Assets/SEE/Cinemachines/CinemachinesRoot.cs
Comment thread Assets/SEE/Cinemachines/CinemachinesRoot.cs
Comment thread Assets/SEE/Cinemachines/CinemachineSceneRoot.cs
Comment thread Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs Outdated
Comment thread Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs Outdated
Comment thread Assets/Resources/UI/PictureInPicture/PIPCamera.uxml
Comment thread Assets/Resources/UI/Cinemachines/ControlCameraView.uxml Outdated
Comment thread Assets/Resources/UI/Cinemachines/ControlCameraDataSource.asset
Comment thread Assets/Resources/UI/PictureInPicture/PIP.asset
ThePlays001 and others added 2 commits July 29, 2026 11:35
added missing / to the <see cref> tag

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 42 out of 60 changed files in this pull request and generated 5 comments.

Files not reviewed (18)
  • Assets/Cinemachines.meta: Generated file
  • Assets/Cinemachines/CinemachinesMainOutput.renderTexture.meta: Generated file
  • Assets/Cinemachines/CinemachinesPIPOutput.renderTexture.meta: Generated file
  • Assets/Cinemachines/Scenes.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/ControlCamera.prefab.meta: Generated file
  • Assets/Resources/Prefabs/Cinemachines/SceneTemplate.prefab.meta: Generated file
  • Assets/Resources/UI.meta: Generated file
  • Assets/Resources/UI/Cinemachines.meta: Generated file
  • Assets/Resources/UI/Cinemachines/ControlCamera.asset.meta: Generated file
  • Assets/Resources/UI/Cinemachines/ControlCameraDataSource.asset.meta: Generated file
  • Assets/Resources/UI/Cinemachines/ControlCameraView.uxml.meta: Generated file
  • Assets/Resources/UI/PictureInPicture.meta: Generated file
  • Assets/Resources/UI/PictureInPicture/RenderTextures.meta: Generated file
  • Assets/Resources/UI/PictureInPicture/RenderTextures/rtpip_blank.renderTexture.meta: Generated file
  • Assets/Resources/UI/PictureInPicture/pip-camera.uxml.meta: Generated file
Suppressed comments (18)

Assets/SEE/UI/PictureInPicture/pipDataSource.cs:17

  • HeaderAttribute targets fields, but this attribute currently targets the auto-property and causes CS0592. Explicitly target the generated backing field, as is already done for SerializeField.
    Assets/SEE/UI/PictureInPicture/pipDataSource.cs:23
  • The exported property is named PIPImage, but both UXML bindings use the case-sensitive path pipImage, and both assets serialize <pipImage>k__BackingField instead of <PIPImage>k__BackingField. Consequently the bindings cannot resolve this property and the initial texture values do not deserialize. Align the casing in both UXML and asset files with this property.
    Assets/SEE/Cinemachines/CinemachineSceneRoot.cs:179
  • The non-editor branch still references child, but the loop variable is Child. Player compilation therefore fails with an undefined identifier despite the earlier switch to Destroyer.Destroy.
    Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs:325
  • SetParent defaults to worldPositionStays: true. The child was just populated with its serialized local transform while it had no parent, so preserving that temporary world transform changes its local position/rotation/scale when reparented. Use false to retain the restored local transform.
    Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs:616
  • FindObjectsByType searches all loaded scenes, despite this method's per-scene contract. With additive scenes, valid one-root-per-scene setups are treated as duplicates, and the save loop writes the same arbitrary root under every loaded scene's backup key. Accept/pass the target Scene and filter results by root.gameObject.scene before selecting or counting roots.
    Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs:102
  • Validation accepts a first sector whose start is greater than zero, but GetSplinePosition unconditionally selects SpeedList[0]; that speed is then applied before its declared start. Require the first sector to start at zero so the whole normalized spline has a valid initial sector.
    Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs:43
  • Sector boundaries are defined in normalized [0,1) coordinates, but this implementation ignores _positionUnit and compares them directly with currentPosition. If the CinemachineSplineDolly uses Distance or Knot units, sector selection is wrong. Convert the current position to normalized units for lookup (then return in the requested unit), or explicitly reject non-normalized units in validation/configuration.
    Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab:266
  • The PIP brain also enables an AudioListener while the desktop player already supplies the scene listener. This creates multiple active listeners whenever the Cinemachines root is present; disable this cinematic listener.
  m_Enabled: 1

Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs:143

  • rootObject is a Transform, so this stores the Transform's ID rather than the GameObject's ID; that same Transform ID is already mapped through StoredComponent. No GameObject ID is ever serialized, so fields referencing a GameObject cannot be redirected to the restored object. Store rootObject.gameObject.GetInstanceID() here and add its old-to-new mapping during deserialization.
    Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs:42
  • This return documentation still describes a paused component, but PauseWhenInactive was removed and the implementation now returns early solely when deltaTime <= 0. Update the contract so callers are not told about behavior that no longer exists.
    Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs:745
  • This constructor overload treats the string as the parameter name, so the exception reports rootGameObject cannot be null. as a parameter identifier rather than producing the intended standard message. Pass nameof(rootGameObject).
    Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs:59
  • This message says the list must be longer than one entry, but the implementation accepts exactly one entry. Report that at least one entry is required so the exception accurately describes the validation rule.
    Assets/SEE/UI/PictureInPicture/pipDataSource.cs:15
  • Unity requires a ScriptableObject script filename to match its class name. This class is PIPDataSource, but the file is pipDataSource.cs, so Unity cannot reliably associate the MonoScript and the two new .asset instances can appear as missing scripts. Rename the file (and move its .meta) to PIPDataSource.cs.

This issue also appears in the following locations of the same file:

  • line 17
  • line 23
    Assets/SEE/Cinemachines/CinemachineSceneRoot.cs:30
  • Unity requires a MonoBehaviour script filename to match its class name. The file is CinemachineSceneRoot.cs, while the component is CinemachinesScene; this can leave the component in SceneTemplate.prefab as a missing script and prevent new scene components from being attached. Rename the script and its .meta to match the class (or rename the class and all references).
    Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab:327
  • Both Cinemachine brains have ChannelMask: -1, so they consider the exact same virtual-camera set and select the same highest-priority camera. The Picture-in-Picture output therefore cannot show an independent perspective. Assign distinct brain channel masks and expose/assign the corresponding output channel when creating cameras.
  ChannelMask: -1

Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/CinemachinesBrains.prefab:96

  • This enabled listener is instantiated alongside the existing enabled listener on DesktopPlayer.prefab (line 226) and the other new Cinemachine cameras. Unity supports only one active AudioListener, so play mode logs errors and audio listener selection is ambiguous. Cinematic output cameras should not enable their own listener.

This issue also appears on line 266 of the same file.

  m_Enabled: 1

Assets/Resources/Prefabs/Cinemachines/CinemachinesRoot/ControlCamera.prefab:96

  • The control camera adds another enabled AudioListener even though DesktopPlayer.prefab already has one. Instantiating the root therefore creates multiple active listeners; the display-only control camera should leave audio listening to the player camera.
  m_Enabled: 1

Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs:38

  • The nested <paramref> tag is not self-closing, so this XML documentation is malformed and generates documentation parsing warnings. Close it with />.

This issue also appears in the following locations of the same file:

  • line 42
  • line 43
  • line 59
  • line 98

Comment thread Assets/SEE/Cinemachines/Dolly/SimpleSpeedController.cs Outdated
Comment on lines +10 to +13
using UnityEditor;
using UnityEngine.Splines;
using UnityEditor.Splines;
using UnityEditor.Timeline;

@koschke koschke Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed a critical issue, which will cause our build to fail. All Editor code must be moved to the assembly SEEEditor. See the files contained therein (SEE\Assets\Editor\).

Always make sure that a build can be created when proposing a pull request.

@ThePlays001 ThePlays001 Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Problem is, that this is used inside the Component CinemachinesSceneRoot and Unity doesn't allow for MonoBehaviours to be stored inside the Editor directory.

@ThePlays001 ThePlays001 Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will just wrap all UnityEngine-related elements inside a #if UNITY_EDITOR pre-prossesing declarative, including any components, that only need to exist inside the Editor.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, please document that in the code accordingly. Thanks.

Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs
Comment thread Assets/SEE/Cinemachines/CinemachinesRoot.cs Outdated
Comment thread Assets/SEE/Cinemachines/CinemachinesRoot.cs Outdated
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed
Comment thread Assets/SEE/Cinemachines/Utility/CinemachinesUtility.cs Fixed

@koschke koschke left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

I fixed a few minor coding issues myself. Always make sure that there is a \n at the end of every Log.Debug* message. Whenever possible use the simpler new() instead of new T().
Please take a look at our coding style rules documented here

There are still a few things to do, however. See the latest comments of Copilot.

@koschke

koschke commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

I asked Claude Code to check our coding style rules documented here. Here is its review. Please take a look, @ThePlays001

Reviewing against the guidelines.

Reviewed all 9 new C# files (~1,730 lines) against the guidelines.

Review: 922-bachelor-thesis-better-demo-videos

Build-breaking — should be resolved before style

The SEE runtime assembly now depends on Editor-only assemblies. SEE.asmdef gained six references; three are Editor-only:

Added reference includePlatforms
Unity.Cinemachine all
Unity.Cinemachine.Editor Editor
Unity.Splines all
Unity.Splines.Editor Editor
Unity.Timeline all
Unity.Timeline.Editor Editor

pipDataSource.cs also starts lower-case (as does the asset pipDataSource.asset). For MonoBehaviours the mismatch is more than cosmetic — SceneTemplate.prefab references CinemachineSceneRoot.cs by GUID so it resolves today, but "Add Component" won't find the class and the mismatch is a standing hazard.

Visibility

Guideline: prefer private, then internal; public only when another assembly needs it. The classes themselves are correctly internal — good — but the data surface is public throughout:

pipDataSource.cs:15 · SimpleSpeedController.cs:14,20,22,23,29 · CinemachinesRoot.cs:199 · CinemachineSceneRoot.cs:140,249 · CinemachinesUtility.cs:47,49,50,57,60,61,64,65,66,73,76,77,78,82

Note PIPDataSource and SimpleSpeedController are instantiated through Unity serialization, which needs public fields but not public types — the types can be internal.

Coding

Missing curly brackets (required even for a single statement): SimpleSpeedController.cs:65–67 (two nested ifs) · CinemachineSceneRoot.cs:236–239 · CinemachinesUtility.cs:528–530,603–604,649–650,664–665,667–668,670–671,673–674

new T() instead of new(): CinemachinesRoot.cs:226,331 · CinemachinesUtility.cs:110,141,150,185,190,204,377,466 (new() is used correctly at CinemachinesUtility.cs:66,78,82,98,101,266,398,425,432 and CinemachinesRoot.cs:144 — so this is inconsistency rather than ignorance of the rule.)

Missing \n at end of log messages — roughly 20 occurrences: CinemachinesRoot.cs:158,161,175,251,279,284,289,320 · CinemachineSceneRoot.cs:175,178,187,194,199,204,207,237,239,285 · CinemachinesUtility.cs:584,587,676 Some do comply (CinemachinesRoot.cs:79,362,366,370, CinemachinesUtility.cs:316), so it reads as inconsistency.

var — only one use, CinemachinesUtility.cs:198 foreach (var kvp in storedReferences). That is exactly the sanctioned dictionary key/value exception. Compliant.

Empty Update() methodsCinemachinesRoot.cs:98–101 and CinemachineSceneRoot.cs:123–126. Not a guideline item, but Unity invokes these every frame on every instance for no benefit; delete them.

Commented-out codeSimpleSpeedController.cs:93,99,108,114,120 · CinemachinesRoot.cs:29,74,386 · CinemachineSceneRoot.cs:218 · CinemachinesUtility.cs:519–521.

Formatting

Opening brace on the same line (Ctrl-K F would fix): SimpleSpeedController.cs:89 · CinemachinesUtility.cs:492,635

Missing space after keyword: CinemachinesRoot.cs:116 if(, :359 switch( · CinemachinesUtility.cs:661 if(

Column-aligned assignments that VS formatting will collapse: CinemachinesRoot.cs:296–304 · CinemachinesUtility.cs:19–30,144–158

Trailing whitespace, EOF newlines, and line endings are all cleangit diff --check reports nothing in .cs files, and .gitattributes normalizes to LF in the index. The trailing-whitespace hits in the diff are all Unity-generated .meta/.prefab files, which is normal.

Done well

The namespace handling is exemplary and matches the guideline precisely: Cinemachines.cs, Dolly.cs, Utility.cs and PictureInPicture.cs each carry a <summary> immediately before the namespace keyword with an empty body, and the directory layout mirrors the namespaces exactly. All code sits under SEE, third-party code was not touched, the top-level classes are internal, and most methods do carry <summary>, <param>, <returns> and <exception>.

Other correctness issues worth raising in review

Beyond the guidelines, four things I'd want the author to look at:

SimpleSpeedController.cs:109,115,121 pass "SectorRange"/"SectorSpeed" as ArgumentException's second argument, which is paramName — neither is a parameter name. :52–59 throws IndexOutOfRangeException with the message "must be longer than one (1) entry" while the guard is >= 1; message, doc and check disagree, and ArgumentException would be the apt type. CinemachinesUtility.cs:208 declares a local storedReferences that shadows the field at :185, making the field unreachable in that scope. CinemachinesUtility.cs:307 calls storedReferences.Add unguarded while the structurally identical referenceList.Add at :312 is wrapped in try/catch for duplicate keys.

Also minor: all nine .cs.meta files contain only two lines and lack the MonoImporter block, so Unity will rewrite them on next import and produce diff churn.

koschke and others added 3 commits August 12, 2026 09:57
…-better-demo-videos

# Conflicts:
#	Packages/manifest.json
…DITOR' pre-processor directive and moved the CinemachinesStateRestorer into its own file inside 'SEEEditor' to clarify that it is only for the Editor.
Comment on lines +296 to +299
catch (Exception)
{
Debug.LogWarning($"Attempted to Map '{readComponent.GetInstanceID()}' to a References List, while already having one accosiated to it.\n");
}
Comment on lines +306 to +309
catch (Exception)
{
Debug.LogWarning($"Attempted to Map '{storedComponent.InstanceID}' to '{readComponent.GetInstanceID()}'\n");
}
Comment on lines +212 to +221
if (referenceList.ContainsKey(storedReference.InstanceID))
{
// This part is not working correctly, since Unity differentiates InstanceIDs
// from actual Objects and References
objectInstanceID = referenceList[storedReference.InstanceID];
}
else
{
objectInstanceID = storedReference.InstanceID;
}

// List of Children inside this GameObject
[SerializeReference]
public List<StoredGameObject> ChildGameObjects = new();
/// <summary>
/// Temporary Dictionary for storing InstanceIDs of Objects with their respective StoredProperties.
/// </summary>
private Dictionary<int, List<StoredReference>> storedReferences = new();
/// <summary>
/// Reference List between original InstanceIDs and restored IDs.
/// </summary>
private Dictionary<int, int> referenceList = new();
bool SplineAutoDolly.ISplineAutoDolly.RequiresTrackingTarget => false;

[Tooltip("List of Sections on a Spline, with its corresponding Speeds, inwhich that section needs to be paced with.")]
private SplineSector[] SpeedList = {};
Comment on lines +462 to +468
foreach (string scene in restorableScenes)
{
if (scene != "")
{
stringBuilder.AppendFormat("* {0}\n", scene);
}
}
Comment on lines +313 to +319
foreach (StoredGameObject childStoredGameObject in storedGameObject.ChildGameObjects)
{
Transform child = Deserialize(childStoredGameObject);

// reposition accordingly
child.SetParent(restoredGameObject.transform);
}
{
// ... else get the Object by InstanceID and apply it
int objectInstanceID;
if (referenceList.ContainsKey(storedReference.InstanceID))
…irectives to clarify, that these components and classes only work inside the Unity-Editor. Also correctly tagged the associated GameObjects to 'EditorOnly' during creation for the same reason.
@ThePlays001

Copy link
Copy Markdown
Collaborator Author

This is odd.... When I run the Build Process locally it does build the Project (both a Linux and Windows binary), but not through the CI. According to the logs the CI gives out, it is not able to find the 'UserSettings' Object in the Scene.
Is it possible, that the CI is not building from the SEEStart Scene?

@koschke

koschke commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

@ThePlays001 The CI build is currently not working for other reasons. It's not related to your changes. Just ignore that.

Let me know whether you are happy with your improvements and I'll give them another review. Thanks.

@ThePlays001

ThePlays001 commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

i'm currently debating, whether it is better to put these changes into its own .asmdef-File and include it inside the SEE_Editor.asmdef-File. Generally, i'm happy with the current iteration of changes.

Edit:
I tested out this method and I can confirm, that it does not work, because it will see any script added to the SEE_Editor.asmdef-File as part of the Editor folder, which prevents MonoBehaviour-derived Scripts to be created, even if these scripts and the contents of the new .asmdef-file are in a different folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bachelor or master topic Something that could be worked on in a bachelor or master thesis. enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide better means for recording demo videos (SEE movies)

3 participants