feat(ship-factory): implement module rotation functionality#78
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 154 files, which is 4 over the limit of 150. To get a review, narrow the scope: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (146)
📒 Files selected for processing (154)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds module rotation end-to-end: rotation math, rotated footprint bounds and containment, rotation-aware grid snapping and legality, overlay transform sync, UI rotation buttons and input handling, and tests for rotation and snapping. ChangesModule Rotation Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/Scripts/Core/Constants/GameplayConstants.cs`:
- Line 15: Remove the unused public constant CannonProjectileSpeedMultiplier
from GameplayConstants by deleting the declaration (public const float
CannonProjectileSpeedMultiplier = 1000f;) if it isn't needed; alternatively if
this value belongs to cannon/projectile logic, relocate the declaration into the
relevant cannon or projectile-specific constants/class (e.g., CannonConstants or
ProjectileConstants) and update any future references there so GameplayConstants
no longer contains that unrelated symbol.
In `@Assets/Scripts/ShipFactory/ModuleRotationUtility.cs`:
- Around line 52-59: The normalization in ApplyQuarterTurn miscomputes for large
negative deltaSteps; change the newQuarterTurns calculation to use a
double-modulo pattern (e.g. newQuarterTurns = ((currentQuarterTurns +
deltaSteps) % 4 + 4) % 4) so the result is always 0..3, then apply the rotation
as before (bundle.Instance.transform.localRotation = Quaternion.Euler(0f, 0f,
newQuarterTurns * 90f)); keep using CalculateQuarterTurns to get
currentQuarterTurns.
In `@Assets/Scripts/ShipFactory/ShipFactoryCanvasController.cs`:
- Around line 424-425: The rotation buttons currently use RotateActiveModule
which operates on _draggedModuleBundle ?? _selectedModuleBundle, but that can
differ from the module shown in the info panel (hovered-only context); update
the controller so canRotateCurrentContext is derived from the actual controller
context (i.e., true only when the info panel is showing a dragged or selected
module, not when it's showing hover-only info) and either disable/hide rotation
controls in that hover-only state or make RotateActiveModule early-return when
the panel is in hover-only mode; implement a single helper (e.g.,
GetCurrentContextBundle or IsPanelShowingSelectableModule) used by both the UI
enable/visibility logic and RotateActiveModule to ensure the buttons and action
always target the same bundle (_draggedModuleBundle or _selectedModuleBundle)
and never act when only a hovered module is displayed.
In `@Assets/Scripts/ShipFactory/UI/Runtime/OverlayManager.cs`:
- Around line 72-75: The method SyncTransformFromBundle currently returns
silently when _bundleToOverlay.TryGetValue(bundle, out var overlay) fails;
change this to fail fast by throwing an exception (e.g. throw new
InvalidOperationException($"No overlay found for bundle: {bundle}") or
ArgumentException) instead of returning, so the missing overlay invariant
surfaces immediately; update the SyncTransformFromBundle method to replace the
early return with an exception and keep the rest of the method using the overlay
variable as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bc0b978-ce2e-45e6-a2a6-7666d14e7149
📒 Files selected for processing (20)
Assets/DesignSystem/Docs/COMPONENTS.mdAssets/Scripts/Core/Constants/GameplayConstants.csAssets/Scripts/ShipFactory/LegalPositionCalculator/Calculator.csAssets/Scripts/ShipFactory/ModuleOverlay.csAssets/Scripts/ShipFactory/ModuleRotationUtility.csAssets/Scripts/ShipFactory/ModuleRotationUtility.cs.metaAssets/Scripts/ShipFactory/ShipFactory.ussAssets/Scripts/ShipFactory/ShipFactory.uxmlAssets/Scripts/ShipFactory/ShipFactoryCanvasController.csAssets/Scripts/ShipFactory/ShipFactoryController.csAssets/Scripts/ShipFactory/Snapper.csAssets/Scripts/ShipFactory/Tests/LegalPositionCalculator/CalculatorTests.csAssets/Scripts/ShipFactory/Tests/ModuleRotationUtilityTests.csAssets/Scripts/ShipFactory/Tests/ModuleRotationUtilityTests.cs.metaAssets/Scripts/ShipFactory/Tests/ShipFactory.Tests.asmdefAssets/Scripts/ShipFactory/Tests/SnapperTests.csAssets/Scripts/ShipFactory/Tests/SnapperTests.cs.metaAssets/Scripts/ShipFactory/UI/Runtime/OverlayManager.csAssets/Scripts/ShipFactory/UI/ToolkitComponents/ModuleInfo.csREADME.md
… for improved UI consistency
…nd enhance module info panel layout
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Assets/Scripts/ShipFactory/UI/ToolkitComponents/NotificationPopup.cs (1)
34-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCancel the previous hide timer before scheduling a new one.
Back-to-back
Show(...)calls can hide the newer popup too early because the older scheduled callback still executes.Proposed fix
public class NotificationPopup { @@ private readonly VisualElement _actionPopup; private readonly VisualElement _actionPopupIcon; private readonly Label _actionPopupLabel; + private IVisualElementScheduledItem _hideJob; @@ public void Show(string message, PopupLevel level = PopupLevel.Info) { + _hideJob?.Pause(); + _actionPopup.RemoveFromClassList(ToastInfoClassName); _actionPopup.RemoveFromClassList(ToastWarningClassName); _actionPopup.RemoveFromClassList(ToastDangerClassName); @@ _actionPopupLabel.text = message; _actionPopup.style.display = DisplayStyle.Flex; - _actionPopup.schedule.Execute(() => { _actionPopup.style.display = DisplayStyle.None; }).StartingIn(1600); + _hideJob = _actionPopup.schedule.Execute(() => + { + _actionPopup.style.display = DisplayStyle.None; + _hideJob = null; + }).StartingIn(1600); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Assets/Scripts/ShipFactory/UI/ToolkitComponents/NotificationPopup.cs` around lines 34 - 65, The Show method can leave a previously scheduled hide callback running; add a private field (e.g. IVisualElementScheduledItem _hideSchedule) and before scheduling the new hide call cancel/stop the previous schedule (e.g. _hideSchedule?.Pause() or Dispose()/Cancel() depending on your UIElements API) to prevent older timers from hiding the newly shown popup, then assign the result of _actionPopup.schedule.Execute(...).StartingIn(1600) to _hideSchedule so it can be cancelled on the next Show; update Show (and declare the new _hideSchedule field) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Assets/DesignSystem/Resources/UI/Styles/DesignSystem/Controls.uss`:
- Around line 369-373: The selectors that currently target ".ds-scroll
.unity-base-slider__tracker" (and the sibling rules at the other locations) are
too broad and should be limited to the injected scroller internals; update each
rule to scope through the scroller's internal container (for example change
".ds-scroll .unity-base-slider__tracker" to ".ds-scroll .ds-scroller__content
.unity-base-slider__tracker" or the project’s specific scroller-internal class)
so only sliders inside the scroller internals are restyled; apply the same
scoping change to the corresponding thumb/track selectors at the other
occurrences (lines referenced in the comment) so all three rule sets are
restricted to the scroller internals.
In `@Assets/Scripts/ShipFactory/HoverMarqueeLabel.cs`:
- Around line 53-56: The overflow gating logic in HoverMarqueeLabel.cs is
inverted: instead of starting the marquee only when text truly exceeds the clip
width, the code uses "if (textWidth < clipWidth - OverflowEpsilon) return;"
which can start scrolling when it shouldn't; change the check to compare with a
tolerance the other way (for example: "if (textWidth <= clipWidth +
OverflowEpsilon) return;") so that MeasureTextSize(_label.text, ...) stored in
textWidth will only trigger marquee when textWidth is greater than clipWidth
beyond the OverflowEpsilon threshold.
---
Outside diff comments:
In `@Assets/Scripts/ShipFactory/UI/ToolkitComponents/NotificationPopup.cs`:
- Around line 34-65: The Show method can leave a previously scheduled hide
callback running; add a private field (e.g. IVisualElementScheduledItem
_hideSchedule) and before scheduling the new hide call cancel/stop the previous
schedule (e.g. _hideSchedule?.Pause() or Dispose()/Cancel() depending on your
UIElements API) to prevent older timers from hiding the newly shown popup, then
assign the result of _actionPopup.schedule.Execute(...).StartingIn(1600) to
_hideSchedule so it can be cancelled on the next Show; update Show (and declare
the new _hideSchedule field) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 240f0ec7-8023-40d5-b746-badba8e06c61
⛔ Files ignored due to path filters (1)
Assets/SOs/Modules/Command/CommandModule 1.assetis excluded by!**/*.asset
📒 Files selected for processing (12)
Assets/DesignSystem/Docs/COMPONENTS.mdAssets/DesignSystem/Resources/UI/Styles/DesignSystem/Cards.ussAssets/DesignSystem/Resources/UI/Styles/DesignSystem/Controls.ussAssets/DesignSystem/Resources/UI/Styles/DesignSystem/Navigation.ussAssets/Scripts/ShipFactory/HoverMarqueeLabel.csAssets/Scripts/ShipFactory/HoverMarqueeLabel.cs.metaAssets/Scripts/ShipFactory/ModulePaletteController.csAssets/Scripts/ShipFactory/ShipFactory.ussAssets/Scripts/ShipFactory/ShipFactory.uxmlAssets/Scripts/ShipFactory/UI/ToolkitComponents/ModuleInfo.csAssets/Scripts/ShipFactory/UI/ToolkitComponents/NotificationPopup.csAssets/Scripts/ShipFactory/UI/ToolkitComponents/Resources.cs
✅ Files skipped from review due to trivial changes (2)
- Assets/Scripts/ShipFactory/HoverMarqueeLabel.cs.meta
- Assets/DesignSystem/Docs/COMPONENTS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Assets/Scripts/ShipFactory/UI/ToolkitComponents/ModuleInfo.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #72
resolves #79
Summary by CodeRabbit
New Features
Gameplay
Placement & Reliability
UI / UX
Design System
Tests