Broke Fog of war manager to vision manager and fog of war node#43
Conversation
WalkthroughThis update introduces a new fog of war visualization system for a Godot-based RTS framework, centering around a new Changes
Sequence Diagram(s)sequenceDiagram
participant GameScene
participant VisionManager
participant FogOfWarManager
participant UnitVisionData
participant Minimap
GameScene->>VisionManager: Initialize and set up vision parameters
GameScene->>FogOfWarManager: Initialize and link to VisionManager
VisionManager->>UnitVisionData: Create/Update per-unit vision data
FogOfWarManager->>VisionManager: Request fog texture
VisionManager-->>FogOfWarManager: Provide fog texture for rendering
Minimap->>VisionManager: Request vision data and fog texture
VisionManager-->>Minimap: Provide vision data and fog texture
Minimap->>UnitVisionData: Access unit minimap circle info
Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🔭 Outside diff range comments (5)
addons/rts_framework/features/vision/unit_vision_data_weak_ref.gd (1)
1-39: 🧹 Nitpick (assertive)Add class documentation for better code clarity.
This class would benefit from documentation explaining its purpose, usage patterns, and how it fits into the vision system architecture.
+## A weak reference wrapper for UnitVisionData objects. +## +## This class provides safe access to UnitVisionData properties without maintaining +## a strong reference that would prevent garbage collection. It's used by systems +## that need to reference vision data without owning it. extends WeakRef class_name UnitVisionDataWeakRefdemo/game.tscn (1)
137-141: 🧹 Nitpick (assertive)Scene‑hard reference duplicates the VisionManager path
Both the FogOfWar and the Minimap serialise a NodePath to
../RTSController/VisionManager.
If the hierarchy changes you must fix two places. Consider exporting the property as aNodereference in each script and assign it via the editor’s “Node” picker – this keeps the correct reference even after moving nodes.addons/rts_framework/features/vision/vision_manager.gd (3)
22-29: 🧹 Nitpick (assertive)
valuevsmap_mesh_node– possible slip & missing null‑checks
Inside the setter you rely on the parametervalue(value.mesh.size) instead of the already‑assignedmap_mesh_node. This is OK but makes the code harder to read and may throw ifvalue.meshisnull.
Recommend replacing with the assigned field and adding a guard forvalue.mesh == null:if value and value.mesh: - fog_size = value.mesh.size + fog_size = value.mesh.sizeA quick early‑return would make it even clearer.
30-39:⚠️ Potential issue
fog_sizegetter returns aVector3but the property is declaredVector2i
map_mesh_node.mesh.sizeyields aVector3. Converting a 3‑component vector to aVector2isilently drops the Z component, which may not be the intent and can break tooling relying on a 2‑D size.
Either:
- Return only the X/Z components explicitly (
Vector2i(size.x, size.z)), or- Promote
fog_sizeitself toVector3iif height ever matters.
Failing to reconcile the types will surface as editor warnings or subtle bugs.
89-104: 🛠️ Refactor suggestionDictionary mutated while iterating – will raise at runtime
for mapped_unit in _unit_to_vision_data:iterates over the dictionary keys, yet_cleanup_mappingcallserase, mutating the very dictionary mid‑loop. In Godot 4 this throws: "Dictionary was modified during iteration".
Fix by iterating over a copy:-for mapped_unit in _unit_to_vision_data: +# Make a snapshot to avoid 'modified during iteration' errors +for mapped_unit in _unit_to_vision_data.keys(): if not mapped_unit in units_synced: _cleanup_mapping(mapped_unit)
♻️ Duplicate comments (1)
addons/rts_framework/features/vision/fog_of_war.gd (1)
36-46: Identical problem for debug properties
debug_texture_viewanddebug_texture_view_sizeinherit the two issues above (recursion + missingmaterial_overridecheck). Refactor accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
addons/rts_framework/features/vision/fog_of_war.gd(1 hunks)addons/rts_framework/features/vision/fog_of_war.tscn(1 hunks)addons/rts_framework/features/vision/fog_of_war_manager.gdshader(0 hunks)addons/rts_framework/features/vision/fog_of_war_manager.gdshader.uid(0 hunks)addons/rts_framework/features/vision/fog_of_war_manager.tscn(0 hunks)addons/rts_framework/features/vision/minimap.gd(2 hunks)addons/rts_framework/features/vision/minimap.tscn(3 hunks)addons/rts_framework/features/vision/unit_vision_data.gd(1 hunks)addons/rts_framework/features/vision/unit_vision_data.gd.uid(1 hunks)addons/rts_framework/features/vision/unit_vision_data_weak_ref.gd(1 hunks)addons/rts_framework/features/vision/unit_vision_data_weak_ref.gd.uid(1 hunks)addons/rts_framework/features/vision/vision_manager.gd(6 hunks)addons/rts_framework/features/vision/vision_manager.gd.uid(1 hunks)addons/rts_framework/features/vision/vision_manager.tscn(1 hunks)demo/game.tscn(6 hunks)
💤 Files with no reviewable changes (3)
- addons/rts_framework/features/vision/fog_of_war_manager.gdshader.uid
- addons/rts_framework/features/vision/fog_of_war_manager.gdshader
- addons/rts_framework/features/vision/fog_of_war_manager.tscn
🧰 Additional context used
🧠 Learnings (1)
addons/rts_framework/features/vision/minimap.gd (1)
Learnt from: amitnos123
PR: rluders/rts-framework#35
File: addons/rts_framework/features/vision/minimap.gd:40-53
Timestamp: 2025-04-04T19:36:28.105Z
Learning: The minimap implementation in addons/rts_framework/features/vision/minimap.gd has been tested with over 100 units and performed well without noticeable performance issues.
🔇 Additional comments (18)
addons/rts_framework/features/vision/vision_manager.tscn (4)
6-35: Shader implementation looks goodThe blur shader implementation is well structured with appropriate uniforms for customization. The ability to toggle between screen texture and node texture sampling provides good flexibility.
90-98: Check for potential sizing issue with BlurrOverlayThe BlurrOverlay has offset_right and offset_bottom values of -100.0, making it smaller than its parent viewport. This might be intentional, but verify that this won't cause any rendering issues with the blur effect at the edges.
100-101:Details
✅ Verification successful
VisibilityField may need additional configuration
The VisibilityField Area3D node is added but doesn't have any collision shapes or specific configuration. Ensure that the VisionManager script properly configures this node for detecting visibility changes.
🏁 Script executed:
#!/bin/bash # Check how VisibilityField is configured in the VisionManager script rg -A 5 -B 2 "VisibilityField" --type gdscriptLength of output: 79
I’m not seeing any references to
VisibilityFieldin scripts due to thegdscripttype error. Let’s search both the scene and any GDScript files directly:
🏁 Script executed:
#!/bin/bash # Search for VisibilityField in the scene file rg -n "VisibilityField" -g "*.tscn" # Search for any references in GDScript files rg -n "VisibilityField" -g "*.gd"Length of output: 481
🏁 Script executed:
#!/bin/bash # Inspect how visibility_shape_3d is created and configured in VisionManager rg -A 10 -B 10 "visibility_shape_3d" -g "addons/rts_framework/features/vision/vision_manager.gd"Length of output: 4849
VisibilityField is configured at runtime in VisionManager
TheVisionManager.gdscript dynamically instantiates theVisibilityShape3DPackedScene, sets itsshape.radiusto the unit’s sight range, and adds it as a child of theVisibilityFieldArea3D node. This ensures the field has the proper collision shape for visibility detection—no additional setup is needed in the TSCN.
80-89:Details
✅ Verification successful
Verify the purpose of the hidden Revealer node
The Revealer ColorRect is set to be invisible by default. Its purpose isn't immediately clear from the scene structure alone. Make sure its usage is properly documented in the VisionManager script.
🏁 Script executed:
#!/bin/bash # Check for references to the Revealer node in the codebase rg -A 2 -B 2 "Revealer" --type gdscriptLength of output: 72
🏁 Script executed:
#!/bin/bash # Locate the VisionManager script fd -e gd vision_manager.gd # Search for any references to the Revealer node across .gd and .tscn files rg -n 'Revealer' -C3 --glob '*.{gd,tscn}'Length of output: 2223
The Revealer node’s purpose is already documented in VisionManager.gd
TheRevealerColorRect is tied to the exportedrevel_fogproperty, with a clear comment and setter/getter controlling its visibility. No additional documentation is required.• addons/rts_framework/features/vision/vision_manager.gd@42–47:
– “## Revels the whole fog.” comment
–@export var revel_fogsetter/getter callingfind_child("Revealer")
• addons/rts_framework/features/vision/vision_manager.gd@71:
–@onready var _revealer : ColorRect = find_child("Revealer")addons/rts_framework/features/vision/unit_vision_data_weak_ref.gd.uid (1)
1-2: UID file looks goodThis UID file correctly provides a unique identifier for the UnitVisionDataWeakRef resource.
addons/rts_framework/features/vision/vision_manager.gd.uid (1)
1-2: UID file looks goodThis UID file correctly provides a unique identifier for the VisionManager script.
addons/rts_framework/features/vision/unit_vision_data.gd.uid (1)
1-1: Resource identifier file looks good.This UID file provides a stable reference ID for the new
UnitVisionDataresource class, which is part of the refactored vision system architecture.addons/rts_framework/features/vision/minimap.tscn (4)
1-1: Scene load steps updated to reflect new components.The increased load steps count (from 5 to 6) correctly accounts for the new viewport texture resource added to the scene.
13-15: New viewport texture added for fog of war rendering.This new viewport texture resource is properly configured to reference the SubViewport that will render the fog of war overlay.
45-45: FogOfWarTexture now uses the new viewport texture.The FogOfWarTexture has been properly connected to the new SubViewport through the ViewportTexture resource, supporting the modular fog of war rendering approach.
51-56: Added SubViewport with black background for fog of war rendering.This addition properly separates the fog of war rendering into its own viewport, which aligns with the modular design goal of splitting fog of war functionality from vision management.
addons/rts_framework/features/vision/fog_of_war.tscn (1)
1-29: Fog of War implementation uses efficient shader-based approach.The new FogOfWar scene effectively implements fog of war visualization using a shader-based approach with a MeshInstance3D. Notable aspects:
- Uses a custom shader with configurable parameters for appearance
- Employs a flipped quad mesh that will render over the game world
- Optimizes rendering with disabled shadow casting and appropriate cull margin
- Provides debug visualization options
This implementation follows best practices for efficient fog of war rendering in 3D games.
addons/rts_framework/features/vision/unit_vision_data_weak_ref.gd (2)
1-3: Good use of weak references for vision data.Using WeakRef as a base class is appropriate for managing references to vision data without preventing garbage collection.
37-38: 🧹 Nitpick (assertive)Add return type annotation to the factory method.
The create method is missing a return type annotation, which would improve code clarity.
-static func create(unit_vision_data : UnitVisionData) -> UnitVisionDataWeakRef: +static func create(unit_vision_data : UnitVisionData) -> UnitVisionDataWeakRef: return weakref(unit_vision_data)Also consider adding documentation to explain the purpose of this factory method.
Likely an incorrect or invalid review comment.
addons/rts_framework/features/vision/fog_of_war.gd (1)
22-32: Getter/Setter asymmetry forfog_colorandouter_margin_for_fade_outThe setters never store
value, so the getters will always read from the shader even ifmaterial_overrideisnull.
Store the value in a local field and gate the shader call behind amaterial_overridecheck to avoidInvalid call. Non‑existent function ‘set_shader_parameter’ ....demo/game.tscn (1)
104-107:fog_sizeis serialised but script may override itConfirm that
VisionManagerdoes not overwritefog_sizein_ready().
If it does the value saved here is wasted noise in the scene file.addons/rts_framework/features/vision/vision_manager.gd (2)
126-137: Colour naming swap & parent choice might be inverted
Comment saysshroud_circleis a white circle, yet you assignfog_circle_color(dark). The variables look swapped:shroud_circle.color = fog_circle_color # probably fow_circle_color? ... fow_circle.color = shroud_circle_colorAdditionally,
fow_circleis added as a sibling of_fog_viewport_container. If the intent is to draw into the fog viewport, use_fog_viewport.add_child(fow_circle); otherwise the circle may not render where you expect.Would you confirm the intended colour semantics and render parents?
175-187: 🧹 Nitpick (assertive)
get_fog_texture()can be simplified & null‑safe
The method contains two nearly identical branches and may still returnnull. A concise, safe version:func get_fog_texture() -> ViewportTexture: - var fog_texture_result - if combined_viewport: - fog_texture_result = combined_viewport.get_texture() - else: - fog_texture_result = $CombinedViewport.get_texture() - if fog_texture_result: - return fog_texture_result - return null + if combined_viewport: + return combined_viewport.get_texture() + return nullThis removes dead code and makes intent clearer.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
demo/game.tscn (2)
37-38: Hard‑wiredCombinedViewportpath is fragileThe texture relies on a node called
CombinedViewportexisting at the exact relative path.
If someone renames or restructures theVisionManagerinternals, the reference silently breaks and the material turns black.Consider exposing the texture from
VisionManagervia a getter (as you already do withget_fog_texture()) and assign it from script instead of serialising aNodePath.
110-110:skeletonstill points to a non‑Skeleton3Dnode
MeshInstance3D.skeletonexpects aSkeleton3D, but the path is../RTSController(a plainNode).
At runtime this spams warnings and disables GPU skinning.Either clear the property or reference an actual
Skeleton3D.addons/rts_framework/features/vision/minimap.gd (1)
13-13: Initialise_vision_datato an empty dictionaryThe variable is declared but not given a default value.
Accessing it from other scripts before_ready()would yieldnulland crash on iteration.-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
addons/rts_framework/features/vision/fog_of_war.gd(1 hunks)addons/rts_framework/features/vision/minimap.gd(2 hunks)addons/rts_framework/features/vision/unit_vision_data.gd(1 hunks)addons/rts_framework/features/vision/vision_manager.gd(6 hunks)demo/game.tscn(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
addons/rts_framework/features/vision/minimap.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#35
File: addons/rts_framework/features/vision/minimap.gd:40-53
Timestamp: 2025-04-04T19:36:28.105Z
Learning: The minimap implementation in addons/rts_framework/features/vision/minimap.gd has been tested with over 100 units and performed well without noticeable performance issues.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.215Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
addons/rts_framework/features/vision/unit_vision_data.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/unit_vision_data.gd:4-9
Timestamp: 2025-04-19T19:24:21.696Z
Learning: In Godot, classes that extend Resource cannot use @export annotations for their properties, unlike classes that extend Node.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.550Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
addons/rts_framework/features/vision/vision_manager.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.550Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:18-21
Timestamp: 2025-04-19T19:42:33.849Z
Learning: In the VisionManager class, `texture_units_per_world_unit` is a central parameter that other nodes read from directly, rather than being updated through setter propagation. This acts as a single source of truth for pixel-to-world unit conversion.
addons/rts_framework/features/vision/fog_of_war.gd (1)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/fog_of_war.gd:55-59
Timestamp: 2025-04-19T19:12:10.486Z
Learning: In Godot, `await` requires a signal or coroutine as its target. To wait for a node to be ready, use `await node_reference.ready` instead of `await node_reference`.
🔇 Additional comments (2)
addons/rts_framework/features/vision/unit_vision_data.gd (1)
129-145: Local vs. global position forshape_3d
shape_3d.positionsets the local translation relative to its parent.
If the shape is not parented directly under the world root, its position will drift from the unit when the hierarchy moves.Prefer
global_position(orglobal_transform.origin) to keep the collider aligned in world space:- if shape_3d: - shape_3d.position = unit_pos_3d + if shape_3d: + shape_3d.global_position = unit_pos_3daddons/rts_framework/features/vision/vision_manager.gd (1)
128-136: Colours swapped between shroud and fog circlesThe comment says
shroud_circleshould be white andfow_circlegrey, but the assignments are reversed.- shroud_circle.color = fog_circle_color # Set color + shroud_circle.color = shroud_circle_color ... - fow_circle.color = shroud_circle_color + fow_circle.color = fog_circle_color
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
addons/rts_framework/features/vision/vision_manager.gd (2)
26-33:⚠️ Potential issueInfinite recursion in
map_mesh_nodesetter – crashes on first assignmentThe setter assigns to
map_mesh_nodeinside the setter, which calls itself again and causes a stack overflow.-var map_mesh_node : MeshInstance3D: - set(value): - map_mesh_node = value +var _map_mesh_node : MeshInstance3D +@export var map_mesh_node : MeshInstance3D: + set(value): + _map_mesh_node = value + if _map_mesh_node and _map_mesh_node.mesh: + fog_size = _map_mesh_node.mesh.size + get: + return _map_mesh_node
34-44:⚠️ Potential issue
fog_sizesetter also self‑recursesThe same pattern appears here. Introduce a backing field to break the recursive loop.
-var fog_size : Vector2i = DEFAULT_SIZE: - set(value): - var map_effective_size = value * texture_units_per_world_unit - find_child("CombinedViewport").size = map_effective_size - fog_size = value +var _fog_size : Vector2i = DEFAULT_SIZE +@export var fog_size : Vector2i = DEFAULT_SIZE: + set(value): + _fog_size = value + var map_effective_size = _fog_size * texture_units_per_world_unit + find_child("CombinedViewport").size = map_effective_size get: - if map_mesh_node: - if map_mesh_node.mesh: - return map_mesh_node.mesh.size - return fog_size + if map_mesh_node and map_mesh_node.mesh: + return map_mesh_node.mesh.size + return _fog_size
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
addons/rts_framework/features/vision/fog_of_war.gd(1 hunks)addons/rts_framework/features/vision/unit_vision_data.gd(1 hunks)addons/rts_framework/features/vision/vision_manager.gd(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
addons/rts_framework/features/vision/fog_of_war.gd (1)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/fog_of_war.gd:55-59
Timestamp: 2025-04-19T19:12:10.486Z
Learning: In Godot, `await` requires a signal or coroutine as its target. To wait for a node to be ready, use `await node_reference.ready` instead of `await node_reference`.
addons/rts_framework/features/vision/unit_vision_data.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/unit_vision_data.gd:4-9
Timestamp: 2025-04-19T19:24:21.696Z
Learning: In Godot, classes that extend Resource cannot use @export annotations for their properties, unlike classes that extend Node.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.550Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
addons/rts_framework/features/vision/vision_manager.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.550Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:18-21
Timestamp: 2025-04-19T19:42:33.849Z
Learning: In the VisionManager class, `texture_units_per_world_unit` is a central parameter that other nodes read from directly, rather than being updated through setter propagation. This acts as a single source of truth for pixel-to-world unit conversion.
🔇 Additional comments (1)
addons/rts_framework/features/vision/vision_manager.gd (1)
132-145: Possible wrong parent forfow_circle– verify viewport capture
_fog_viewport_container.add_sibling(fow_circle)places the circle outside the fog SubViewport hierarchy, meaning it may never be rendered into the fog texture.If the intent is to draw both circles into the same viewport, add the second circle to
_fog_viewportinstead:- _fog_viewport_container.add_sibling(fow_circle) + _fog_viewport.add_child(fow_circle)Please confirm that the current node graph still produces the desired texture; otherwise visual artefacts will appear.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
addons/rts_framework/features/vision/vision_manager.gd (2)
27-33: 🛠️ Refactor suggestionInfinite recursion in
map_mesh_nodesetterAssigning to
map_mesh_nodeinside its own setter re‑enters the setter and crashes with a stack‑overflow.-var map_mesh_node : MeshInstance3D: - set(value): - map_mesh_node = value # <- recursive +var _map_mesh_node : MeshInstance3D +@export var map_mesh_node : MeshInstance3D: + set(value): + _map_mesh_node = value + if _map_mesh_node and _map_mesh_node.mesh: + fog_size = _map_mesh_node.mesh.size get: - if map_mesh_node: - if map_mesh_node.mesh: - fog_size = value.mesh.size + return _map_mesh_node
34-43:⚠️ Potential issue
fog_sizesetter has the same recursion bug
fog_size = valueinside the setter calls itself endlessly.
Introduce a backing field (e.g.,_fog_size) or convert the property to a plain field if you don’t need custom logic on assignment.
♻️ Duplicate comments (1)
addons/rts_framework/features/vision/unit_vision_data.gd (1)
102-103: Remove the no‑op setter to makepositiontruly read‑onlyLeaving an empty setter silently discards assignments and can mislead future maintainers into thinking the value is writable.
Delete thesetblock entirely so the API clearly communicates thatpositionis read‑only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
addons/rts_framework/features/vision/fog_of_war.gd(1 hunks)addons/rts_framework/features/vision/unit_vision_data.gd(1 hunks)addons/rts_framework/features/vision/vision_manager.gd(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
addons/rts_framework/features/vision/fog_of_war.gd (1)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/fog_of_war.gd:55-59
Timestamp: 2025-04-19T19:12:10.486Z
Learning: In Godot, `await` requires a signal or coroutine as its target. To wait for a node to be ready, use `await node_reference.ready` instead of `await node_reference`.
addons/rts_framework/features/vision/vision_manager.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.550Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:18-21
Timestamp: 2025-04-19T19:42:33.849Z
Learning: In the VisionManager class, `texture_units_per_world_unit` is a central parameter that other nodes read from directly, rather than being updated through setter propagation. This acts as a single source of truth for pixel-to-world unit conversion.
addons/rts_framework/features/vision/unit_vision_data.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/unit_vision_data.gd:4-9
Timestamp: 2025-04-19T19:24:21.696Z
Learning: In Godot, classes that extend Resource cannot use @export annotations for their properties, unlike classes that extend Node.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.550Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
🔇 Additional comments (2)
addons/rts_framework/features/vision/vision_manager.gd (2)
18-24: Nice fix for the backing‑field recursion issue – the new_texture_units_per_world_unitfield prevents the stack overflow that was previously reported.
132-141: Circle colours appear swapped
shroud_circle(the “what units currently see”) is givenfog_circle_color, whilefow_circle(the faded historical vision) is givenshroud_circle_color.
If that’s not intentional, switch the assignments:-shroud_circle.color = fog_circle_color +shroud_circle.color = shroud_circle_color -fow_circle.color = shroud_circle_color +fow_circle.color = fog_circle_color
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
addons/rts_framework/features/vision/fog_of_war.gd(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
addons/rts_framework/features/vision/fog_of_war.gd (1)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/fog_of_war.gd:55-59
Timestamp: 2025-04-19T19:12:10.486Z
Learning: In Godot, `await` requires a signal or coroutine as its target. To wait for a node to be ready, use `await node_reference.ready` instead of `await node_reference`.
🔇 Additional comments (6)
addons/rts_framework/features/vision/fog_of_war.gd (6)
5-15: Well-implemented vision_manager property with proper backing field.The vision_manager property is correctly implemented with a backing field and appropriate null check in the setter. This prevents the recursion issues highlighted in previous reviews and ensures that operations on the vision manager are safe.
67-76: Good implementation of fog_texture property with backing field.The fog_texture property correctly implements the backing field pattern as recommended in previous reviews. This ensures the property maintains its value even if the material changes at runtime.
78-85: Proper null-checking in _ready method.The _ready method now correctly checks if vision_manager is assigned before attempting to await its ready signal, and includes a helpful warning message when it's not assigned. This addresses previous issues with invalid await targets.
87-92: Safe implementation of _apply_fog_texture_from_vision_manager.The method properly guards against a null _vision_manager reference before attempting to use it, preventing potential crashes at runtime.
19-26: Fixed recursive assignment in texture_units_per_world_unit property.The property now correctly uses a backing field (_texture_units_per_world_unit) to store the value, preventing the infinite recursion that would occur when assigning to the property within its own setter.
20-20: 🧹 Nitpick (assertive)Redundant default value assignment in property declaration.
The property declaration assigns the same default value (2) that's already set in the backing field. One of these assignments can be removed to avoid redundancy.
-@export_range(1, 10000, 1,"suffix:px/length") var texture_units_per_world_unit : int = 2 : # px/length +@export_range(1, 10000, 1,"suffix:px/length") var texture_units_per_world_unit : int : # px/lengthLikely an incorrect or invalid review comment.
… on minimap. Example draw alert of been attacked in the minimap.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
addons/rts_framework/features/vision/vision_manager.tscn (2)
48-53: Fixed viewport size hard‑codes the resolution
The 500×500CombinedViewportrepeats the configurability concern raised earlier. Expose width/height as exported variables or derive them fromfog_sizeto support different map sizes and aspect ratios.
77-79:EditorOnlyCirclestill visible in buildsPrevious review suggested hiding or removing this helper outside the editor. No runtime guard was added here.
Wrap its visibility inif not Engine.is_editor_hint(): … hide()insidevision_manager.gd.demo/game.tscn (2)
111-111: MeshInstance3Dskeletonset to non‑Skeleton node – this issue was flagged before and still persists. Remove the property or point it to an actualSkeleton3Dto avoid warnings and fallback to CPU skinning.
37-49:⚠️ Potential issue
ViewportTexturepath is wrong – fog will render black
CombinedViewportlives insideRTSController/VisionManager, but theViewportTexture_fqft5(and the mirror at lines 51‑53) points toCombinedViewportat the scene root, so the shader will sample an empty viewport.-viewport_path = NodePath("CombinedViewport") +viewport_path = NodePath("../RTSController/VisionManager/CombinedViewport")Apply the same fix to
ViewportTexture_ri1tv.addons/rts_framework/features/vision/minimap.gd (1)
11-14: Initialise_vision_datato avoidnullaccess
_vision_datais declared but uninitialised until_ready().
Any script that queries the minimap earlier will hitnull.-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
addons/rts_framework/features/vision/minimap.gd(2 hunks)addons/rts_framework/features/vision/vision_manager.tscn(3 hunks)demo/game.tscn(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
addons/rts_framework/features/vision/minimap.gd (1)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.215Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
addons/rts_framework/features/vision/minimap.gd (1)
85-93: 🛠️ Refactor suggestionGracefully handle missing
CombinedViewportto avoid orphaned nodes
draw_node_on_minimap()returnsfalsewhen_minimap_viewportis missing, yet the caller always proceeds to store the circle inUnitVisionData.
This leaves a liveDynamicCircle2Din memory that will never be drawn and is hard to clean up.func draw_node_on_minimap(draw_node : CanvasItem) -> bool: if _minimap_viewport: _minimap_viewport.add_child(draw_node) return true - return false + + draw_node.queue_free() # Prevent leakage + push_error("CombinedViewport not found; minimap circle discarded for '" + name + "'.") + return false(Also a tiny typo: the doc-string header says “Addes” → “Adds”.)
♻️ Duplicate comments (1)
addons/rts_framework/features/vision/minimap.gd (1)
11-14:⚠️ Potential issueEagerly initialise
_vision_datato prevent null-dereference races
_physics_process()assumes_vision_datais never null (for unit in _vision_data) yet the variable is only assigned in_ready().
Although Godot guarantees_readyruns before the first_physics_process, other scripts can legitimately access this class before_readyfires (e.g. viaSceneTreelook-ups); they would hit a fatal “attempt to iterate on a null instance” error.-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>This mirrors the pattern we discussed in the last PR and provides immediate, safe default state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
addons/rts_framework/features/vision/minimap.gd(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
addons/rts_framework/features/vision/minimap.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#35
File: addons/rts_framework/features/vision/minimap.gd:40-53
Timestamp: 2025-04-04T19:36:28.105Z
Learning: The minimap implementation in addons/rts_framework/features/vision/minimap.gd has been tested with over 100 units and performed well without noticeable performance issues.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.215Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
|
@rluders I implemented both |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
addons/rts_framework/features/vision/minimap.gd (2)
11-14: 🧹 Nitpick (assertive)Initialize
_vision_dataeagerly and document its content
_vision_datais declared without a default value.
Because it is accessed by several helper methods before every frame, it is safer to start with an empty dictionary and document the expected key/value types.-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>This prevents accidental
nulldereferences should another script poke the minimap before_ready()finishes loading.
81-84: 🧹 Nitpick (assertive)Apply the same world-to-minimap scale used elsewhere
The circle radius is currently set in world units. On a typical 200-px-wide minimap a 30-metre sight range becomes a 30-pixel circle, often dwarfing the map.
Consider scaling by the engine-wide
texture_units_per_world_unit(or another dedicated constant) so the minimap remains legible:var pixels_per_unit := vision_manager.texture_units_per_world_unit minimap_circle.radius = unit.get_sight_range() * pixels_per_unit(or clamp to a fixed radius if that suits your UX better).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
addons/rts_framework/features/vision/minimap.gd(2 hunks)addons/rts_framework/features/vision/vision_manager.gd(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
addons/rts_framework/features/vision/vision_manager.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.579Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:18-21
Timestamp: 2025-04-19T19:42:33.892Z
Learning: In the VisionManager class, `texture_units_per_world_unit` is a central parameter that other nodes read from directly, rather than being updated through setter propagation. This acts as a single source of truth for pixel-to-world unit conversion.
addons/rts_framework/features/vision/minimap.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#35
File: addons/rts_framework/features/vision/minimap.gd:40-53
Timestamp: 2025-04-04T19:36:28.105Z
Learning: The minimap implementation in addons/rts_framework/features/vision/minimap.gd has been tested with over 100 units and performed well without noticeable performance issues.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.240Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
🔇 Additional comments (1)
addons/rts_framework/features/vision/vision_manager.gd (1)
104-106: Good catch on stale‐mapping cleanupSwitching to
keys()and adding thenot mapped_unit.is_revealing()clause guarantees that vanished or hidden units are purged in one place, leveraging theUnitVisionDataauto-cleanup.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
addons/rts_framework/features/vision/minimap.gd (2)
13-14:⚠️ Potential issueInitialize
_vision_datato an empty dictionary to avoidnulldereferences
_vision_datais declared without a default value, so it isnulluntil_ready()succeeds.
Several code-paths (e.g._physics_process()) access.keys()on this field and will crash if_ready()bails out early (see lines 44-47).-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>
82-84: 🧹 Nitpick (assertive)Sight range is in world units – consider applying the same scale factor used elsewhere
Minimap circles are currently created with a radius equal to the unit’s real sight range (e.g. 30 m ⇒ 30 px).
If your minimap is only a few hundred pixels wide, large sight ranges dominate the view.
Scaling byvision_manager.texture_units_per_world_unitkeeps visual proportions consistent with the fog-of-war and 3 D visibility logic.- minimap_circle.radius = unit.get_sight_range() + minimap_circle.radius = unit.get_sight_range() * vision_manager.texture_units_per_world_unitaddons/rts_framework/features/vision/vision_manager.gd (1)
18-24: Exported property shows “0” in the inspector even though the runtime default is 2Because
texture_units_per_world_unitlacks an initialiser, the inspector serialises a value of0; newcomers may assume “0 px/length” is valid.
Giving the exported property the same default as the backing field avoids confusion:-@export_range(1, 10000, 1,"suffix:px/length") var texture_units_per_world_unit : int : +@export_range(1, 10000, 1,"suffix:px/length") var texture_units_per_world_unit : int = 2 :
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
addons/rts_framework/features/vision/minimap.gd(2 hunks)addons/rts_framework/features/vision/vision_manager.gd(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
addons/rts_framework/features/vision/minimap.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#35
File: addons/rts_framework/features/vision/minimap.gd:40-53
Timestamp: 2025-04-04T19:36:28.105Z
Learning: The minimap implementation in addons/rts_framework/features/vision/minimap.gd has been tested with over 100 units and performed well without noticeable performance issues.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.240Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
addons/rts_framework/features/vision/vision_manager.gd (3)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:138-147
Timestamp: 2025-04-19T19:18:47.579Z
Learning: UnitVisionData is a Resource class that automatically cleans up its associated nodes (fow_circle, shroud_circle, minimap_circle, shape_3d) when deleted by implementing the _notification method with NOTIFICATION_PREDELETE.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:18-21
Timestamp: 2025-04-19T19:42:33.892Z
Learning: In the VisionManager class, `texture_units_per_world_unit` is a central parameter that other nodes read from directly, rather than being updated through setter propagation. This acts as a single source of truth for pixel-to-world unit conversion.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:175-187
Timestamp: 2025-05-07T18:20:17.897Z
Learning: In VisionManager, the `combined_viewport` property can be null during async loading of nodes or before _ready() completes, so fallback mechanisms should be maintained when accessing it.
🔇 Additional comments (1)
addons/rts_framework/features/vision/minimap.gd (1)
55-60: 🧹 Nitpick (assertive)Iterate safely when the vision data may mutate during the frame
_vision_data.keys()returns a snapshot, but ifVisionManagerclears the dictionary between this call and theforloop,unitmay no longer exist in_vision_data, causing look-ups in_unit_is_minimap_mapped()to fail.A tiny defensive tweak:
for unit in _vision_data.keys().duplicate():(or skip the second membership test by caching the value).
⛔ Skipped due to learnings
Learnt from: amitnos123 PR: rluders/rts-framework#43 File: addons/rts_framework/features/vision/minimap.gd:11-15 Timestamp: 2025-04-19T19:15:02.240Z Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
addons/rts_framework/features/vision/minimap.gd (1)
11-14: Initialize_vision_datato an empty dictionary
_vision_datais declared but not initialized. Accessing it from any script that runs before_ready()(e.g. an autoload querying the minimap tree) will raise a null-reference error.-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>addons/rts_framework/features/vision/vision_manager.gd (1)
18-24: Inspector shows 0 while getter returns 2 – consider explicit defaultWithout an initializer the editor lists
texture_units_per_world_unitas 0, which is misleading even though the getter returns the backing value2.
Setting an explicit default keeps editor & runtime in sync:-@export_range(1, 10000, 1,"suffix:px/length") var texture_units_per_world_unit : int : +@export_range(1, 10000, 1,"suffix:px/length") var texture_units_per_world_unit : int = 2 :
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
addons/rts_framework/features/vision/minimap.gd(2 hunks)addons/rts_framework/features/vision/vision_manager.gd(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
addons/rts_framework/features/vision/vision_manager.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:18-21
Timestamp: 2025-04-19T19:42:33.892Z
Learning: In the VisionManager class, `texture_units_per_world_unit` is a central parameter that other nodes read from directly, rather than being updated through setter propagation. This acts as a single source of truth for pixel-to-world unit conversion.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/vision_manager.gd:175-187
Timestamp: 2025-05-07T18:20:17.897Z
Learning: In VisionManager, the `combined_viewport` property can be null during async loading of nodes or before _ready() completes, so fallback mechanisms should be maintained when accessing it.
addons/rts_framework/features/vision/minimap.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.240Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:38-47
Timestamp: 2025-05-07T18:32:27.790Z
Learning: In the minimap implementation for the RTS framework, we should disable physics processing with `set_physics_process(false)` when initialization fails (like when vision data is null), rather than adding runtime checks in the `_physics_process()` method, to avoid affecting runtime performance.
🔇 Additional comments (3)
addons/rts_framework/features/vision/minimap.gd (1)
38-48: Graceful fallback logic looks solidNice job short-circuiting initialisation when
vision_managerfails and disabling physics processing – this avoids per-frame null checks and keeps runtime perf unchanged. 👍addons/rts_framework/features/vision/vision_manager.gd (2)
104-106: Dictionary-mutation race fixed – good catchIterating over
_unit_to_vision_data.keys()preventsERR_CHANGEDwhen the dictionary is pruned during the same pass.
Thanks for hardening this edge-case!
130-139: Colour swap corrected
shroud_circlenow usesshroud_circle_colorandfow_circleusesfog_circle_color, matching their intended semantics. ✔️
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
addons/rts_framework/features/vision/minimap.gd (1)
11-14: 🛠️ Refactor suggestionInitialize
_vision_dataeagerly to avoidnull-dereference edge-cases
_vision_datais declared but not initialised until_ready().
Any code path (editor plug-ins, tool scripts, unit tests, or even another node’s_ready) that accesses the minimap before_ready()runs will still seenulland crash on the first dictionary call.-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>A zero-cost default removes the hazard without affecting the later assignment from
vision_manager.get_vision_data().
(This was raised in earlier reviews but is still outstanding.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
addons/rts_framework/features/vision/minimap.gd(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
addons/rts_framework/features/vision/minimap.gd (3)
Learnt from: amitnos123
PR: rluders/rts-framework#35
File: addons/rts_framework/features/vision/minimap.gd:40-53
Timestamp: 2025-04-04T19:36:28.105Z
Learning: The minimap implementation in addons/rts_framework/features/vision/minimap.gd has been tested with over 100 units and performed well without noticeable performance issues.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.240Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:38-47
Timestamp: 2025-05-07T18:32:27.790Z
Learning: In the minimap implementation for the RTS framework, we should disable physics processing with `set_physics_process(false)` when initialization fails (like when vision data is null), rather than adding runtime checks in the `_physics_process()` method, to avoid affecting runtime performance.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
addons/rts_framework/features/vision/minimap.gd (2)
11-14: Initialize_vision_datato an empty dictionary
_vision_datais declared but not initialised.
Any script that tries to access the minimap before_ready()finishes (e.g. an autoload queryingMinimap.get_vision_data()) will hit anullreference.A one-liner keeps the property always valid:
-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>
85-88: 🧹 Nitpick (assertive)Map world-space sight range to minimap pixels
Using
unit.get_sight_range()as a pixel radius often produces circles larger than the minimap.
If that’s intentional, feel free to ignore; otherwise scale it withvision_manager.texture_units_per_world_unit(which you already have at hand):- minimap_circle.radius = unit.get_sight_range() + minimap_circle.radius = unit.get_sight_range() * vision_manager.texture_units_per_world_unit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
addons/rts_framework/features/vision/minimap.gd(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
addons/rts_framework/features/vision/minimap.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.240Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:38-47
Timestamp: 2025-05-07T18:32:27.790Z
Learning: In the minimap implementation for the RTS framework, we should disable physics processing with `set_physics_process(false)` when initialization fails (like when vision data is null), rather than adding runtime checks in the `_physics_process()` method, to avoid affecting runtime performance.
🔇 Additional comments (1)
addons/rts_framework/features/vision/minimap.gd (1)
58-64: 🧹 Nitpick (assertive)Minor micro-optimisation – cache
_vision_data.keys()
_vision_data.keys()allocates a new array every frame.
For large armies this shows up in the profiler.var units := _vision_data.keys() for unit in units: ...A tiny optimisation, but worth it in tight loops.
⛔ Skipped due to learnings
Learnt from: amitnos123 PR: rluders/rts-framework#43 File: addons/rts_framework/features/vision/minimap.gd:11-15 Timestamp: 2025-04-19T19:15:02.240Z Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
addons/rts_framework/features/vision/minimap.gd (2)
13-14: Initialize_vision_datato an empty dictionary to avoid early-access null crashes
_vision_datais declared but not initialized until_ready(). Any script that queries the minimap before_ready()(e.g. via autoload, editor tool script, unit test) will still seenulland can crash.
This exact concern was raised in a previous review and is still outstanding.-var _vision_data : Dictionary +var _vision_data : Dictionary = {} # Dictionary<BaseEntity, UnitVisionData>
86-89: Consider scaling sight-range to minimap pixelsMapping a 30 m sight range directly to a 30-pixel radius may overflow small minimaps.
If you notice icons dominating the minimap, introduce a conversion constant:const WORLD_TO_MINIMAP : float = 0.1 minimap_circle.radius = unit.get_sight_range() * WORLD_TO_MINIMAPYou’ve previously mentioned the current visuals are acceptable, so treat this as optional polish.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
addons/rts_framework/features/vision/minimap.gd(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
addons/rts_framework/features/vision/minimap.gd (2)
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:11-15
Timestamp: 2025-04-19T19:15:02.240Z
Learning: In the RTS Framework, the `_vision_data` in `minimap.gd` stores direct references to `UnitVisionData` objects, not weak references. It should be initialized as an empty dictionary to prevent null access.
Learnt from: amitnos123
PR: rluders/rts-framework#43
File: addons/rts_framework/features/vision/minimap.gd:38-47
Timestamp: 2025-05-07T18:32:27.790Z
Learning: In the minimap implementation for the RTS framework, we should disable physics processing with `set_physics_process(false)` when initialization fails (like when vision data is null), rather than adding runtime checks in the `_physics_process()` method, to avoid affecting runtime performance.
🔇 Additional comments (1)
addons/rts_framework/features/vision/minimap.gd (1)
37-52: Robust early-exit path looks goodGreat job converting the missing-dependency guard into a hard shutdown that disables both
_physics_process()and_process().
This keeps the node inert in release builds and costs zero per-frame overhead.
|
@rluders I think it's ready |
2 participants
Summary by CodeRabbit
New Features
Improvements
Refactor