Animation timeline - #1147
Conversation
# Conflicts: # Assets/Resources/UnityGLTFSettings.asset # Assets/Scenes/Main.unity # Assets/Scripts/ImportGltfast.cs # Assets/Scripts/UnityGLTF Plugins/OpenBrushExportPlugin.cs # Assets/Scripts/Widgets/ModelWidget.cs # Assets/Settings/Localization/Strings/Strings Shared Data.asset # Packages/manifest.json # Packages/packages-lock.json
# Conflicts: # .github/workflows/build.yml # Assets/Editor/BuildTiltBrush.cs # Assets/Editor/UiScreenshotter.cs # Assets/Plugins/Android/AndroidManifest.xml # Assets/Prefabs/NoPeekingCamera.prefab # Assets/Scenes/Main.unity # Assets/Scripts/Config.cs # Assets/Scripts/GUI/AdminPanel.cs # Assets/Scripts/GUI/PanelManager.cs # Assets/Scripts/InitNoHeadsetMode.cs # Assets/Scripts/SketchControlsScript.cs # Assets/Scripts/UserConfig.cs # Assets/Settings/Localization/Strings/Strings Shared Data.asset # ProjectSettings/ProjectSettings.asset
Restrict the shared-ring ownership fix to the Ellipse modifier used by QuillEllipse. Legacy shape modifiers keep their established full-chunk processing so existing brushes retain their original geometry.
Carry the exact BrushDescriptor through isolated GLB fixture exports instead of scanning every catalog material by name. This avoids unrelated missing material references aborting deterministic fixture generation while leaving normal exports unchanged.
Use the original Toon and TubeToonInverted materials without replacement overlays, and identify their base and outline passes explicitly for URP. The deterministic gray-background captures now match the legacy renderer exactly for TubeToonInverted and within two pixels for Toon.
Add a deterministic gray-background capture for Toon and TubeToonInverted so dark outlines remain measurable independently of the normal black brush-screenshot background. Use the same 1x resolution and 1x MSAA parity settings as post-processing-disabled captures.
Advance the resolved upm package revision from 50edd44 to 4673c2b so clean checkouts use the latest pushed shader and tooling fixes.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88f07dc426
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| App.Scene.ActiveCanvas = layer; | ||
| NotifyLayerCanvasesUpdate(); | ||
| // Add canvases for other animation frames | ||
| layer = animationUI_manager.AddLayerRefresh(layer); |
There was a problem hiding this comment.
TransformItems.Transform, TransformSelected, and TransformList call AddLayerNow() solely to create a temporary canvas and then destroy it, without restoring ActiveCanvas. These new unconditional side effects therefore leave ActiveCanvas pointing at a destroyed object and register a timeline track whose canvas has been destroyed after every selection/item transform. Use a transient-canvas path for these callers, or explicitly remove the track and restore the previous active canvas.
Useful? React with 👍 / 👎.
| @@ -287,10 +288,20 @@ void LoadModel() | |||
| { | |||
| return; | |||
| } | |||
|
|
|||
| this.gameObject.transform.SetParent(App.Scene.ActiveCanvas.transform); | |||
There was a problem hiding this comment.
When a deferred model finishes loading, OnPacCatalogChanged() assigns Model and reaches this line. If the user has switched animation frames or layers while the asset was downloading, the widget is silently moved from its saved canvas to the currently active canvas; the animation ownership index is not notified either. LoadModel() should instantiate the model contents without changing the widget's parent.
Useful? React with 👍 / 👎.
| _ => throw new ArgumentOutOfRangeException(nameof(path), $"Unsupported audio type: {path}.") | ||
| }; | ||
|
|
||
| using (UnityWebRequest uwr = UnityWebRequestMultimedia.GetAudioClip(path, audioType)) |
There was a problem hiding this comment.
Library entries pass ordinary absolute filesystem paths into LoadClip, but UnityWebRequestMultimedia.GetAudioClip is being given that path directly rather than a file:/// URI. This makes local clips fail as malformed/unsupported URLs on platforms such as Windows; the repository's GltfAudioSource.LoadAndPlay and AudioCaptureManager.LoadAudio already perform the required file-URI conversion.
Useful? React with 👍 / 👎.
| SoundClipController soundClipController = new SoundClipController(this, widget); | ||
| m_Controller = soundClipController; | ||
| SoundClipCatalog.Instance.StartCoroutine(PrepareAudioPlayer(InitializeControllers)); |
There was a problem hiding this comment.
A catalog SoundClip can back multiple widgets, including clones, but every CreateController overwrites the single m_Controller field and both PrepareAudioPlayer and InitializeControllers later dereference that field. If a second widget is created before the first asynchronous load completes, the first coroutine initializes the second widget's controller instead, leaving the first widget permanently uninitialized. Pass the newly created controller through that coroutine/callback rather than storing one shared controller.
Useful? React with 👍 / 👎.
| public SoundClip(string filePath) | ||
| { | ||
| PersistentPath = filePath.Substring(App.SoundClipLibraryPath().Length + 1); | ||
| HumanName = System.IO.Path.GetFileName(PersistentPath); | ||
| AbsolutePath = filePath; |
There was a problem hiding this comment.
SoundClipWidget.FromModelWidget constructs a SoundClip from each GltfAudioSource.AbsoluteFilePath, which normally points into the model's directory rather than SoundClipLibraryPath. Unconditionally slicing at the sound-library prefix length can throw ArgumentOutOfRangeException for shorter paths, or create a corrupted persistent path for longer ones, so breaking apart a glTF model containing audio fails or produces an unreloadable clip. Validate the prefix and copy or otherwise represent external audio paths explicitly.
Useful? React with 👍 / 👎.
|
The following package files were changed: The following files from ProjectSettings were changed: (Review: flagged files) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 498ee81de8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| m_Color = color, | ||
| m_Seed = 0, | ||
| m_ControlPoints = controlPoints.ToArray(), | ||
| m_OverrideColors = controlPointColors[pathIndex].ToList() |
There was a problem hiding this comment.
Every existing API/SVG caller that omits the new optional controlPointColors argument reaches this unconditional index operation with null, causing DrawNestedTrList to throw before creating its first stroke. Preserve the previous behavior by assigning no overrides when the argument is null, and also guard against a list shorter than the generated paths.
Useful? React with 👍 / 👎.
| m_ControlPointColors ??= Enumerable.Repeat((Color32?)null, m_ControlPoints.Count).ToList(); | ||
| m_ControlPointColors.Add(CurrentColorOverride); |
There was a problem hiding this comment.
When color overrides are first enabled, the current control point has already been inserted into m_ControlPoints, so initializing the color list with m_ControlPoints.Count null entries and then appending the current color makes the list one element longer and shifts every override by one index. A saved or recreated stroke therefore renders its first point with the base color and each later point with the previous point's color; update the corresponding entry using the same append-versus-replace decision as the control-point list.
Useful? React with 👍 / 👎.
| m_ControlPointColors, | ||
| CurrentColorOverrideMode |
There was a problem hiding this comment.
For a batched colored stroke, this passes the pointer's mutable m_ControlPointColors list directly into the stored Stroke, while DetachLine clears only m_ControlPoints and never resets the color list. Drawing another colored stroke consequently appends into the previous stroke's saved data and starts the new stroke with all earlier colors; copy the list when memorizing and reset it when the line ends.
Useful? React with 👍 / 👎.
| public static CameraPathMetadata[] GetCameraPaths() | ||
| { | ||
| return WidgetManager.m_Instance.CameraPathWidgets | ||
| return WidgetManager.m_Instance.AllPathWidgets | ||
| .Where(cpw => cpw.WidgetScript.ShouldSerialize()) | ||
| .Select(cpw => cpw.WidgetScript.AsSerializable()) |
There was a problem hiding this comment.
AllPathWidgets includes animation paths, so each active animation path is written into both CameraPaths here and AnimationPaths by GetAnimationPaths(). Loading processes both arrays independently; the duplicate replaces and hides its predecessor through AddAnimationPath, adds spurious undo commands, and the hidden animation path remains eligible for AllPathWidgets, causing the number of serialized path objects to grow on subsequent save/load cycles. Use the non-animation CameraPathWidgets collection for CameraPaths.
Useful? React with 👍 / 👎.
| foreach (var layer in m_Layers) | ||
| { | ||
| layer.gameObject.SetActive(false); | ||
| } |
There was a problem hiding this comment.
When a Quill import is undone, merely deactivating its canvases does not mark their animation tracks or scene layers as deleted. The next timeline frame change sees those tracks as visible and reactivates their canvases, and LayerCanvasesSerialized() still includes the ghost layers in saved sketches. Undo and redo should use the scene layer deletion/restoration paths, as AddLayerCommand does, so timeline and layer indexes remain coherent.
Useful? React with 👍 / 👎.
# Conflicts: # Assets/Environments/NightSky/Materials/NightSky_FloorForeground.mat # Assets/Environments/NightSky/Materials/NightSky_Spikes.mat # Assets/Environments/NightSky/Materials/NightSky_Stage.mat # Assets/Prefabs/Panels/ExtraPanel.prefab # Assets/Prefabs/Panels/LabsPanel.prefab # Assets/Resources/UnityGLTFSettings.asset # Assets/Scenes/Main.unity # Assets/Scripts/API/ApiMethods.cs # Assets/Scripts/API/DrawStrokes.cs # Assets/Scripts/App.cs # Assets/Scripts/Commands/LoadQuillCommand.cs # Assets/Scripts/Quill.cs # Assets/Scripts/Save/MetadataUtils.cs # Assets/Scripts/Save/SketchMetadata.cs # Assets/Scripts/SoundClip.cs # Assets/Scripts/StrokeData.cs # Assets/Scripts/Tools/BaseTool.cs # Assets/Scripts/UnityGLTF Plugins/OpenBrushExportPlugin.cs # Assets/Scripts/WidgetManager.cs # Assets/Settings/Localization/Strings/Strings Shared Data.asset # Assets/Settings/Localization/Strings/Strings_en.asset # Assets/ThirdParty/UnityODS/Scripts/HybridCamera.cs # Packages/manifest.json # Packages/packages-lock.json # ProjectSettings/ProjectSettings.asset
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85953ab239
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (newEntry.layerId, newEntry.frameId) = | ||
| App.Scene.GetSerializableIndexOfCanvas(widget.Canvas); |
There was a problem hiding this comment.
When a snapshot is taken while a model remains selected—notably during autosave, which does not call ClearActiveSelection()—widget.Canvas is the selection canvas, which has no timeline location, so this records (-1, -1). Reloading then falls back to the main canvas and moves the model out of its original animation frame; the newly added frame serializers for images, videos, lights, portals, captures, and sound clips use the same pattern. Resolve the selection canvas through m_PreviousCanvas, as the stroke snapshot path already does.
Useful? React with 👍 / 👎.
| if (createdStrokes == null) App.Scene.DestroyCanvas(canvas); | ||
| else App.Scene.DestroyCanvas(canvas, createdStrokes); |
There was a problem hiding this comment.
When undoing a split or duplicated frame containing widgets, this destroys the cloned canvas and its child widgets directly. SceneScript.DestroyCanvas() only removes strokes and batches, while GrabWidget.OnDestroy() does not unregister the widget or subtract its tilt-meter cost; model/image vertex bookkeeping also relies on UnregisterGrabWidget(). Repeating duplicate/split and undo therefore accumulates dead widget entries and inflated complexity/vertex counts, eventually rejecting otherwise valid content.
Useful? React with 👍 / 👎.
|
|
||
| } | ||
|
|
||
| if (allCollectedStrokes.Count > 0 || createdWidgets.Count > 0) |
There was a problem hiding this comment.
For a renderable Quill top-level layer containing only a 360-degree picture or no drawable content, AddLayerNow() has already added the layer, but the picture path returns no widget and there are no strokes, so this condition skips creation of LoadQuillCommand. The new layer consequently remains active and serializable with no way to undo the merge; either record the command when createdLayers is nonempty or destroy those layers when no content was imported.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8382ad8a73
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (int, int) serializedTimelineLocation = belongsToAnimation && | ||
| App.Scene?.animationUI_manager != null | ||
| ? App.Scene.animationUI_manager.GetSerializableTimelineLocation(timelineLocation) | ||
| : timelineLocation; |
There was a problem hiding this comment.
When a frame containing an animation path is moved, or an earlier frame is inserted or extended, the timeline moves the PathToken but never updates CameraPath.timelineLocation. Serializing that stale field here records the path at its old frame, so reopening the sketch attaches the camera animation to the wrong drawing. Resolve the path's current location from the timeline (or update every affected path location after timeline edits) before serializing.
Useful? React with 👍 / 👎.
| public static CameraPathMetadata[] GetAnimationPaths() | ||
| { | ||
| return WidgetManager.m_Instance.AnimationPathWidgets | ||
| .Select(apw => apw.WidgetScript.AsSerializable()) |
There was a problem hiding this comment.
After deleting a frame with an animation path, the path widget remains active while the delete command is retained for undo; it is only hidden when that undo state is eventually disposed. AnimationPathWidgets therefore still returns it here, even though no current timeline frame references it, so saving before the command leaves the undo stack writes the deleted path and loading the sketch resurrects it at its former location. Serialize only paths referenced by the current timeline.
Useful? React with 👍 / 👎.
1 participant
No description provided.