WorldScape3D - #1329
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds the WorldScape 3D module with terrain data, rendering, collision, instancing, asset management, editor tools, build integration, embedded brush resources, and public API documentation. ChangesWorldScape 3D module
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🔵 Low · up to The null-editor handling can leave stale terrain gizmo geometry visible after the editor is gone. This is a bounded correctness issue that should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant Editor as WorldScape3DEditor
participant UI as WorldScape3DUI
participant Terrain as WorldScape3D
participant Data as WorldScape3DData
participant Renderer as WorldScape3DMesher
Editor->>UI: Select tool and provide brush data
UI->>Editor: Forward operation and viewport input
Editor->>Data: Edit region maps
Data->>Terrain: Emit map and edited-area changes
Terrain->>Renderer: Update terrain mesh and rendering state
Renderer-->>Editor: Refresh visible terrain
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (42)
modules/worldscape_3d/doc_classes/WorldScape3DEditor.xml-118-119 (1)
118-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a description for the
SCULPTconstant.All other
Toolconstants have descriptions.SCULPThas an empty body.SCULPT(1) andHEIGHT(2) are separate values, so readers cannot tell them apart.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DEditor.xml` around lines 118 - 119, Add a descriptive documentation body for the SCULPT constant in the Tool enum, distinguishing it from HEIGHT and explaining its sculpting purpose. Preserve the existing constant name, value, and surrounding documentation structure.modules/worldscape_3d/doc_classes/WorldScape3DMaterial.xml-60-63 (1)
60-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete two member descriptions and fix one grammar error.
auto_shader_enabledanddual_scaling_enabledhave empty descriptions. Line 149 contains "using a the nearest pixel only".✏️ Proposed fix for the grammar error
- Textures are filtered using a the nearest pixel only. It is faster than LINEAR, but the texture will look pixelated. Use this for a low-poly look, with a very low uv_scale. + Textures are filtered using the nearest pixel only. It is faster than LINEAR, but the texture will look pixelated. Use this for a low-poly look, with a very low uv_scale.Also applies to: 148-150
🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DMaterial.xml` around lines 60 - 63, Complete the XML descriptions for the auto_shader_enabled and dual_scaling_enabled members, describing each setting’s behavior consistently with the surrounding WorldScape3DMaterial documentation. In the affected description around the nearest-pixel explanation, remove the extra article so the wording uses “using the nearest pixel only.”modules/worldscape_3d/doc_classes/WorldScape3DData.xml-82-87 (1)
82-87: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winExternal documentation URLs were carried over from the upstream project. The shared root cause is that the rename to WorldScape3D did not update the external link hosts and targets. The links now point to Terrain3D and Godot documentation, and one uses a path form that differs from the other links in this PR.
modules/worldscape_3d/doc_classes/WorldScape3DData.xml#L82-L87: remove or replace theterrain3d.readthedocs.iotips link. The same link also appears on lines 139 and 177.modules/worldscape_3d/doc_classes/WorldScape3DRegion.xml#L111-L115: change thedocs.godotengine.orghost todocs.redotengine.organd remove or replace theterrain3d.readthedocs.iocontrol map link.modules/worldscape_3d/doc_classes/WorldScape3DInstancer.xml#L18-L18: add the.htmlsuffix to theclass_multimeshlink so it matches the other class links.modules/worldscape_3d/doc_classes/WorldScape3DMeshAsset.xml#L9-L9: add the.htmlsuffix to theclass_multimeshlink and point theMultiMeshInstance3Dlabel atclass_multimeshinstance3d.html.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DData.xml` around lines 82 - 87, Update the documentation links in WorldScape3DData.xml lines 82-87, 139, and 177 by removing or replacing the terrain3d.readthedocs.io tips links; in WorldScape3DRegion.xml lines 111-115, change docs.godotengine.org to docs.redotengine.org and remove or replace the terrain3d.readthedocs.io control-map link; in WorldScape3DInstancer.xml line 18, add .html to the class_multimesh link; and in WorldScape3DMeshAsset.xml line 9, add .html to class_multimesh and point MultiMeshInstance3D to class_multimeshinstance3d.html.modules/worldscape_3d/doc_classes/WorldScape3DInstancer.xml-14-14 (1)
14-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix two wording errors.
Line 14 contains "this is can be used procedurally". Lines 74 describe
append_regionas acting "within a region location", but the parameter is aWorldScape3DRegionobject.✏️ Proposed fix
- - [method remove_instances] - Like add_instances, this is can be used procedurally but is designed for hand editing. + - [method remove_instances] - Like add_instances, this can be used procedurally but is designed for hand editing.- Appends new transforms to the existing data within a region location. The mesh_id should already be setup as a [WorldScape3DMeshAsset] in the asset dock. + Appends new transforms to the existing data within the specified region. The mesh_id should already be setup as a [WorldScape3DMeshAsset] in the asset dock.Also applies to: 66-77
🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DInstancer.xml` at line 14, Correct the wording in the WorldScape3DInstancer documentation: revise the remove_instances description to replace “this is can be used” with grammatical text, and update the append_region description to refer to operating on a WorldScape3DRegion object rather than “within a region location.”modules/worldscape_3d/doc_classes/WorldScape3D.xml-44-44 (1)
44-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect two text errors in
get_intersection.Line 44 contains "This operation is does not use physics". Line 64 calls
3.402823466e+38the "maximum double float value". That value is the maximum single-precision float.✏️ Proposed fix
- Casts a ray from [code skip-lint]src_pos[/code] pointing towards [code skip-lint]direction[/code], attempting to intersect the terrain. This operation is does not use physics, so enabling collision is unnecessary. + Casts a ray from [code skip-lint]src_pos[/code] pointing towards [code skip-lint]direction[/code], attempting to intersect the terrain. This operation does not use physics, so enabling collision is unnecessary.- - If there is no intersection, eg. the ray points towards the sky, it returns the maximum double float value [code skip-lint]Vector3(3.402823466e+38F,...)[/code]. You can check this case with this code: [code skip-lint]if point.z > 3.4e38:[/code] + - If there is no intersection, eg. the ray points towards the sky, it returns the maximum single-precision float value [code skip-lint]Vector3(3.402823466e+38F,...)[/code]. You can check this case with this code: [code skip-lint]if point.z > 3.4e38:[/code]Also applies to: 64-64
🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3D.xml` at line 44, Correct the documentation text for get_intersection: change “is does not use physics” to grammatically correct wording, and update the description of 3.402823466e+38 to identify it as the maximum single-precision float rather than the maximum double-precision value.modules/worldscape_3d/doc_classes/WorldScape3DRegion.xml-154-156 (1)
154-156: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the grammar in
vertex_spacing.The text contains "This value is manage by the instancer".
✏️ Proposed fix
- Stored instancer transforms are laterally scaled by this value. This value is manage by the instancer on loading or when [member WorldScape3D.vertex_spacing] is set, and shouldn't be manually adjusted. + Stored instancer transforms are laterally scaled by this value. The instancer manages this value on loading or when [member WorldScape3D.vertex_spacing] is set. Do not adjust it manually.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DRegion.xml` around lines 154 - 156, Update the documentation text for the vertex_spacing member to correct the grammar in “This value is manage by the instancer,” while preserving the existing meaning and surrounding guidance.modules/worldscape_3d/doc_classes/WorldScape3D.xml-150-150 (1)
150-150: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stray backtick.
The line ends with `[method get_intersection].`` The backtick renders in the class reference.
✏️ Proposed fix
- See [method get_intersection].` + See [method get_intersection].🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3D.xml` at line 150, Remove the trailing backtick from the get_intersection method reference in the WorldScape3D documentation, leaving the sentence ending with a period so the backtick is not rendered.modules/worldscape_3d/doc_classes/WorldScape3DData.xml-421-431 (1)
421-431: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse the declared parameter name in the
save_regiondescription.The parameter is
save_16_bit. The description lists it as16_bit.✏️ Proposed fix
- - 16_bit - converts the edited 32-bit heightmap to 16-bit. This is a lossy operation. + - save_16_bit - converts the edited 32-bit heightmap to 16-bit. This is a lossy operation.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DData.xml` around lines 421 - 431, Update the save_region method description to refer to the declared save_16_bit parameter name instead of 16_bit, while preserving the existing explanation of the lossy 32-bit-to-16-bit conversion.modules/worldscape_3d/doc_classes/WorldScape3DInstancer.xml-23-30 (1)
23-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the stale
tool_settings.gdreference.This module implements the tool settings bar in C++ (
modules/worldscape_3d/editor/ui/worldscape_3d_tools.cpp). The description points readers to a GDScript file from the original GDExtension addon.✏️ Proposed fix
- Used during terrain editing to place instances, given many brush parameters. In addition to the brush position, it also uses the following parameters: asset_id, size, strength, fixed_scale, random_scale, fixed_spin, random_spin, fixed_tilt, random_tilt, align_to_normal, height_offset, random_height, vertex_color, random_hue, random_darken. All of these settings are set in the editor through tool_settings.gd. + Used during terrain editing to place instances, given many brush parameters. In addition to the brush position, it also uses the following parameters: asset_id, size, strength, fixed_scale, random_scale, fixed_spin, random_spin, fixed_tilt, random_tilt, align_to_normal, height_offset, random_height, vertex_color, random_hue, random_darken. All of these settings are set in the editor tool settings bar.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DInstancer.xml` around lines 23 - 30, Remove the stale “tool_settings.gd” reference from the add_instances method description in WorldScape3DInstancer documentation, while preserving the list of brush parameters and the existing description of terrain editing behavior.modules/worldscape_3d/doc_classes/WorldScape3DMeshAsset.xml-63-63 (1)
63-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the issue tracker reference.
The URL points to
github.com/godotengine/godot/issues/102799. The link text calls it "Redot issue#102799". Use a label that matches the host, or link the corresponding Redot issue.✏️ Proposed fix
- Currently broken and hidden in the inspector until [url=https://github.com/godotengine/godot/issues/102799]Redot issue `#102799`[/url] is fixed. + Currently broken and hidden in the inspector until [url=https://github.com/godotengine/godot/issues/102799]Godot issue `#102799`[/url] is fixed.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DMeshAsset.xml` at line 63, Correct the issue reference in the documentation text around the WorldScape3DMeshAsset inspector note: either relabel it as a Godot issue to match the existing godotengine URL or update the URL to the corresponding Redot issue while preserving the note’s meaning.modules/worldscape_3d/doc_classes/WorldScape3DData.xml-296-303 (1)
296-303: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the failure return value for
get_roughness.The method returns
float. The description states it returnsColor(NAN, NAN, NAN, NAN).✏️ Proposed fix
- Returns [code skip-lint]Color(NAN, NAN, NAN, NAN)[/code] if the position is outside of defined regions. + Returns [code skip-lint]NAN[/code] if the position is outside of defined regions.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DData.xml` around lines 296 - 303, Update the get_roughness documentation to describe a float failure value rather than Color(NAN, NAN, NAN, NAN), using the appropriate NaN representation for float when the position is outside defined regions.modules/worldscape_3d/doc_classes/WorldScape3DData.xml-85-85 (1)
85-85: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the broken Tips links
The
tips.html#using-the-generated-height-map-in-other-shadersURL returns 404 in all threeWorldScape3DData.xmldescriptions at lines 85, 139, and 177. Replace it with a valid documentation URL or remove the link. Thecontrolmap_format.htmllink inWorldScape3DRegion.xmlresolves successfully.🤖 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 `@modules/worldscape_3d/doc_classes/WorldScape3DData.xml` at line 85, Update the three WorldScape3DData.xml descriptions referencing “using-the-generated-height-map-in-other-shaders” to use a valid documentation URL, or remove those Tips links while preserving the surrounding API descriptions.doc/classes/WorldScape3DAssetResource.xml-1-10 (1)
1-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove
WorldScape3DAssetResource.xmlto the module documentation directory.Add
WorldScape3DAssetResourcetoget_doc_classes(), move the file tomodules/worldscape_3d/doc_classes/, and change the schema path to../../../doc/class.xsd.🤖 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 `@doc/classes/WorldScape3DAssetResource.xml` around lines 1 - 10, Move the WorldScape3DAssetResource documentation file into the module’s doc_classes directory, update its xsi:noNamespaceSchemaLocation to ../../../doc/class.xsd, and add WorldScape3DAssetResource to the module’s get_doc_classes() list.modules/worldscape_3d/worldscape_3d.cpp-1010-1014 (1)
1010-1014: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
get_physics_process_delta_time()in the physics notification.This case handles
NOTIFICATION_PHYSICS_PROCESSbut readsget_process_delta_time(), which returns the idle frame delta. The two values differ when the physics tick rate does not match the frame rate.
__physics_processdoes not currently usep_delta, so behavior does not change today. The value becomes wrong as soon as the parameter is used.🐛 Proposed fix
- __physics_process(get_process_delta_time()); + __physics_process(get_physics_process_delta_time());🤖 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 `@modules/worldscape_3d/worldscape_3d.cpp` around lines 1010 - 1014, Update the NOTIFICATION_PHYSICS_PROCESS branch to pass get_physics_process_delta_time() to __physics_process instead of get_process_delta_time(), preserving the existing physics notification flow.modules/worldscape_3d/worldscape_3d_assets.cpp-266-269 (1)
266-269: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport
normal_mipmapsin the normal-map mismatch message.The message on Line 268 compares against
albedo_mipmaps. The surrounding block validates the normal texture and comparesmipmapsagainstnormal_mipmapson Line 266. The printed reference value is therefore wrong and misleads the user during texture preparation.🐛 Proposed fix
- LOG(ERROR, "Texture ID ", i, " normal mipmap setting (", mipmaps, ") doesn't match first texture (", albedo_mipmaps, "). They must be identical. Read Texture Prep in docs."); + LOG(ERROR, "Texture ID ", i, " normal mipmap setting (", mipmaps, ") doesn't match first texture (", normal_mipmaps, "). They must be identical. Read Texture Prep in docs.");🤖 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 `@modules/worldscape_3d/worldscape_3d_assets.cpp` around lines 266 - 269, Update the mismatch log in the normal-map validation block to report normal_mipmaps as the reference value instead of albedo_mipmaps, while preserving the existing comparison and warning behavior.modules/worldscape_3d/worldscape_3d_assets.cpp-571-571 (1)
571-571: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp each component of
p_sizeseparately.
CLAMPexpands to comparisons withoperator<andoperator>. ForVector2i, those operators compare lexicographically, not per component. A value such asVector2i(5000, 1)compares greater thanVector2i(4096, 4096)and is replaced entirely byVector2i(4096, 4096). Theycomponent changes from1to4096.🐛 Proposed fix
- Vector2i size = CLAMP(p_size, Vector2i(1, 1), Vector2i(4096, 4096)); + Vector2i size = Vector2i(CLAMP(p_size.x, 1, 4096), CLAMP(p_size.y, 1, 4096));🤖 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 `@modules/worldscape_3d/worldscape_3d_assets.cpp` at line 571, Update the size calculation to clamp the x and y components of p_size independently to the [1, 4096] range, rather than applying CLAMP to the Vector2i as a whole; preserve valid component values without allowing either dimension outside the bounds.modules/worldscape_3d/worldscape_3d_util.cpp-63-68 (1)
63-68: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the cast result before dereferencing
obj.Line 64 casts a
VarianttoObject *. Line 65 dereferences the result. AVariantwith typeOBJECTcan hold a null pointer or refer to an object that was already freed. The dereference then crashes.Line 65 runs in all build configurations. Only the
LOGcall on Line 66 is removed in release builds, so the guard cannot be omitted.The same pattern appears in
print_dictat Lines 92-93.🛡️ Proposed fix
case Variant::OBJECT: { Object *obj = cast_to<Object>(var); - String str = "Object#" + String::num_uint64(obj->get_instance_id()) + ", " + ptr_to_str(obj); + String str = obj ? "Object#" + String::num_uint64(obj->get_instance_id()) + ", " + ptr_to_str(obj) : "<null>"; LOG(p_level, i, ": ", str);🤖 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 `@modules/worldscape_3d/worldscape_3d_util.cpp` around lines 63 - 68, Guard the Object* result in the Variant::OBJECT branch before calling get_instance_id() or ptr_to_str(), handling null or invalid references without dereferencing them. Apply the same protection to the corresponding object handling in print_dict, and keep the guard active in all build configurations.modules/worldscape_3d/config.py-13-25 (1)
13-25: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd
WorldScape3DAssetResourceto the module documentation map.
register_types.cppregisters this class, butget_doc_classes()does not map it to the module path.DocTools::save_classestherefore uses the defaultdoc/classespath. Move its XML tomodules/worldscape_3d/doc_classes/and add the class name toget_doc_classes().🤖 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 `@modules/worldscape_3d/config.py` around lines 13 - 25, Update get_doc_classes() to include WorldScape3DAssetResource, matching the class registered in register_types.cpp. Move its XML documentation from the default doc/classes location into modules/worldscape_3d/doc_classes/ so DocTools::save_classes maps it to the module documentation path.modules/worldscape_3d/worldscape_3d_util.h-98-123 (1)
98-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
is_power_of_2constexpr. The math types support the conversion,bilerp, andaabb2rectfunctions. However,is_valid_region_sizecannot be evaluated at compile time becauseis_power_of_2is notconstexpr.🤖 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 `@modules/worldscape_3d/worldscape_3d_util.h` around lines 98 - 123, Make the existing is_power_of_2 function constexpr so is_valid_region_size can be evaluated at compile time, while preserving its current behavior and interface.modules/worldscape_3d/worldscape_3d_mesh_asset.cpp-386-390 (1)
386-390: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe log prints the wrong member.
Line 389 reports the generated size but prints
_generated_faces. Print_generated_size.- LOG(INFO, "Setting generated size: ", _generated_faces); + LOG(INFO, "Setting generated size: ", _generated_size);🤖 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 `@modules/worldscape_3d/worldscape_3d_mesh_asset.cpp` around lines 386 - 390, Update the logging statement in WorldScape3DMeshAsset::set_generated_size to report _generated_size instead of _generated_faces, while leaving the surrounding generated-size update logic unchanged.modules/worldscape_3d/worldscape_3d_mesh_asset.cpp-462-467 (1)
462-467: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
max_rangetruncates the float range to an int.Line 463 assigns the result of a float
CLAMPto anint. A range of31.5becomes31, and the followingCLAMPmixesintandreal_t. Usereal_t.- int max_range = CLAMP(_lod_ranges[1] - _lod_ranges[0], 0.f, 64.f); + real_t max_range = CLAMP(_lod_ranges[1] - _lod_ranges[0], 0.f, 64.f);Line 170 has the same class of problem:
_generated_facesis anintbut the code assigns2.f. Use2.🤖 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 `@modules/worldscape_3d/worldscape_3d_mesh_asset.cpp` around lines 462 - 467, The set_fade_margin method currently truncates the clamped floating-point LOD range by storing it in int max_range; change max_range to real_t so fractional ranges are preserved throughout the fade-margin clamp. Also update the _generated_faces initialization near line 170 to assign the integer literal 2 instead of 2.f.modules/worldscape_3d/worldscape_3d_region.h-53-72 (1)
53-72: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThe parallel constant arrays have no bounds enforcement at their use sites.
FORMAT,TYPESTR, andCOLOReach hold 4 entries, indexed byMapType.WorldScape3DRegion::sanitize_map(worldscape_3d_region.cppLines 178-180) indexes all three withp_map_typeand performs no range check.
sanitize_mapis bound to script at Line 406 of the.cpp.VARIANT_ENUM_CASTdoes not validate the incoming value, so a script can pass any integer. Values of 4 or greater read past the end of all three arrays.
get_mapandset_mapboth use aswitchwith adefaultbranch, so onlysanitize_mapis exposed. Add the same range check there.🛡️ Proposed guard for `worldscape_3d_region.cpp`
Ref<Image> WorldScape3DRegion::sanitize_map(const MapType p_map_type, const Ref<Image> &p_map) const { if (p_map_type < 0 || p_map_type >= TYPE_MAX) { LOG(ERROR, "Requested map type ", p_map_type, ", is invalid"); return Ref<Image>(); } ... }🤖 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 `@modules/worldscape_3d/worldscape_3d_region.h` around lines 53 - 72, Update WorldScape3DRegion::sanitize_map to validate p_map_type before indexing FORMAT, TYPESTR, or COLOR; reject values below zero or at least TYPE_MAX, log the invalid map type, and return an empty Ref<Image>. Preserve the existing sanitization flow for valid MapType values.modules/worldscape_3d/worldscape_3d_material.cpp-882-882 (1)
882-882: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSave failures log the
ERRORlog level instead of the returned error code. Both messages pass theERRORlogging-level constant where theErrorvalue fromResourceSaver::savebelongs. Every save failure therefore reports the same meaningless number, and the accompanying advice to look up the@GlobalScopeErrorenum cannot be followed.
modules/worldscape_3d/worldscape_3d_material.cpp#L882: replace theERRORargument witherr.modules/worldscape_3d/worldscape_3d_region.cpp#L300: replace theERRORargument witherr.🤖 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 `@modules/worldscape_3d/worldscape_3d_material.cpp` at line 882, Replace the logging-level constant with the returned save error variable err in both save-failure messages: modules/worldscape_3d/worldscape_3d_material.cpp:882 and modules/worldscape_3d/worldscape_3d_region.cpp:300. Keep the existing ResourceSaver failure handling and message structure unchanged.modules/worldscape_3d/worldscape_3d_mesher.cpp-79-79 (1)
79-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe grid-type labels in the log are swapped.
The parameter is
p_standard_grid. When it istrue, the message prints "symmetric". When it isfalse, it prints "standard". The two labels are reversed relative to the parameter name and to theMeshTypenames (STANDARD_TILE,STANDARD_EDGE_A).🐛 Proposed fix
- LOG(DEBUG, "Generating vertices and indices for a", p_standard_grid ? " symmetric " : " standard ", "grid mesh of width: ", p_size.x, " and height: ", p_size.y); + LOG(DEBUG, "Generating vertices and indices for a", p_standard_grid ? " standard " : " symmetric ", "grid mesh of width: ", p_size.x, " and height: ", p_size.y);🤖 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 `@modules/worldscape_3d/worldscape_3d_mesher.cpp` at line 79, Correct the conditional labels in the vertex/index generation log so p_standard_grid true reports “standard” and false reports “symmetric,” matching the parameter and MeshType naming. Update only the ternary expression in the LOG call.modules/worldscape_3d/worldscape_3d_collision.cpp-409-412 (1)
409-412: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the shape id, not the grid index, in the diagnostic lookup.
iis the grid cell index. The shape id for that cell isgrid[i]._shape_get_position(i)therefore reports the position of an unrelated shape.
_shape_get_positionalso indexes_shapes[p_shape_id]without a bounds check. Todaygrid.size()equals the shape count because both derive from the samegrid_widthformula. If that ever diverges, this line becomes an out-of-range read.🐛 Proposed fix
if (!p_rebuild && grid[i] >= 0) { - [[maybe_unused]] Vector2i center_pos = v3v2i(_shape_get_position(i)); + [[maybe_unused]] Vector2i center_pos = v3v2i(_shape_get_position(grid[i]));🤖 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 `@modules/worldscape_3d/worldscape_3d_collision.cpp` around lines 409 - 412, Update the diagnostic lookup in the !p_rebuild branch to pass grid[i], the active shape id, to _shape_get_position instead of i, the grid cell index; keep the existing center_pos logging and continuation behavior unchanged.modules/worldscape_3d/worldscape_3d_instancer.cpp-807-808 (1)
807-808: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the multimesh reference before use.
add_multimeshis bound to script at Line 1248.p_multimeshis dereferenced at Line 808 with no validity test. A script call that passesnulldereferences a null pointer.🛡️ Proposed guard
void WorldScape3DInstancer::add_multimesh(const int p_mesh_id, const Ref<MultiMesh> &p_multimesh, const Transform3D &p_xform, const bool p_update) { + if (p_multimesh.is_null()) { + LOG(ERROR, "Provided multimesh is null. Doing nothing."); + return; + } LOG(INFO, "Extracting ", p_multimesh->get_instance_count(), " transforms from multimesh");🤖 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 `@modules/worldscape_3d/worldscape_3d_instancer.cpp` around lines 807 - 808, Update WorldScape3DInstancer::add_multimesh to validate p_multimesh before calling get_instance_count or otherwise dereferencing it. If the Ref<MultiMesh> is null or invalid, return immediately using the surrounding code’s established invalid-reference handling.modules/worldscape_3d/worldscape_3d_instancer.cpp-568-577 (1)
568-577: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
get_mesh_assetresults are dereferenced without a validity test. Both sites validate only that the mesh id is inside the range0toget_mesh_count() - 1. That check does not guarantee the slot holds an asset, soget_mesh_assetcan still return a nullRef._update_mmis(Line 85) and_create_multimesh(Line 422) both test the returned reference before use.
modules/worldscape_3d/worldscape_3d_instancer.cpp#L568-L577: testmesh_asset.is_null()and return before theget_density()call at Line 575.modules/worldscape_3d/worldscape_3d_instancer.cpp#L764-L765: testmesh_asset.is_null()andcontinuebefore theget_height_offset()call; themodifier_shiftbranch iterates every mesh id, so empty slots are reachable.🤖 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 `@modules/worldscape_3d/worldscape_3d_instancer.cpp` around lines 568 - 577, Add validity checks after get_mesh_asset in both _update_mmis (modules/worldscape_3d/worldscape_3d_instancer.cpp:568-577) and _create_multimesh (modules/worldscape_3d/worldscape_3d_instancer.cpp:764-765). Return immediately from _update_mmis when mesh_asset is null, and continue from _create_multimesh before get_height_offset; preserve the existing mesh-id range checks.modules/worldscape_3d/worldscape_3d_instancer.cpp-1218-1226 (1)
1218-1226: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd the initialization guard to
dump_mmis.
dump_mmisis bound to script at Line 1256. Line 1225 calls_terrain->get_mmi_parent()->print_tree()with no check on_terrain.dump_data(Line 1184) guards withIS_DATA_INIT_MESG. Callingdump_mmisbefore initialization dereferences a null pointer.🛡️ Proposed guard
void WorldScape3DInstancer::dump_mmis() { + IS_DATA_INIT_MESG("Instancer isn't initialized.", VOID); LOG(WARN, "Dumping MMI tree and node containers");🤖 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 `@modules/worldscape_3d/worldscape_3d_instancer.cpp` around lines 1218 - 1226, Add the same IS_DATA_INIT_MESG initialization guard used by dump_data to WorldScape3DInstancer::dump_mmis before accessing _terrain or calling get_mmi_parent()->print_tree(). Return immediately when initialization is incomplete, while preserving the existing dump behavior after initialization.modules/worldscape_3d/worldscape_3d_region.cpp-212-225 (1)
212-225: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck
p_mapbefore dereferencing, and test both bounds on one axis.
validate_map_sizeis bound to script at Line 407. Line 213 callsp_map->get_size()with no validity test. A script call withnulldereferences a null pointer.sanitize_mapis the only internal caller and it guards, so the gap is only reachable from script.Line 222 also mixes axes: it compares
region_sizev.xagainst the lower bound andregion_sizev.yagainst the upper bound. The equality check at Line 214 makes this equivalent today. If that check is ever relaxed, a 32 by 4096 image passes both comparisons.🛡️ Proposed fix
bool WorldScape3DRegion::validate_map_size(const Ref<Image> &p_map) const { + if (p_map.is_null()) { + LOG(ERROR, "Provided map is null"); + return false; + } Vector2i region_sizev = p_map->get_size(); @@ - if (region_sizev.x < 64 || region_sizev.y > 2048) { + if (region_sizev.x < 64 || region_sizev.x > 2048) { LOG(ERROR, "Image size out of bounds (64-2048): ", region_sizev); return false; }🤖 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 `@modules/worldscape_3d/worldscape_3d_region.cpp` around lines 212 - 225, Update WorldScape3DRegion::validate_map_size to return false before calling get_size when p_map is invalid or null, protecting the script-bound entry point. Apply both the lower and upper size bounds to the same axis (preferably region_sizev.x), while preserving the square and power-of-two validation.modules/worldscape_3d/worldscape_3d_region.cpp-119-122 (1)
119-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe error message states the wrong expected count.
The condition requires exactly
TYPE_MAXmaps, which is 3. The message printsTYPE_MAX - 1, which is 2. A caller that follows the message supplies 2 maps and the call fails again.🐛 Proposed fix
if (p_maps.size() != TYPE_MAX) { - LOG(ERROR, "Expected ", TYPE_MAX - 1, " maps. Received ", p_maps.size()); + LOG(ERROR, "Expected ", TYPE_MAX, " maps. Received ", p_maps.size()); return; }🤖 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 `@modules/worldscape_3d/worldscape_3d_region.cpp` around lines 119 - 122, Correct the error message in the p_maps validation block to report TYPE_MAX as the expected map count, matching the condition’s requirement for exactly TYPE_MAX maps; leave the validation behavior unchanged.modules/worldscape_3d/worldscape_3d_data.cpp-796-799 (1)
796-799: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCheck the material reference before dereferencing it.
_terrainis checked, butt_materialis not.Ref<>::operator->on a null reference dereferences a null pointer.WorldScape3D::_initializeinstantiates the material only during initialization, andWorldScape3DMaterial::destroycan clear it, so a null material is reachable from a script call.🛡️ Proposed guard
if (_terrain) { Ref<WorldScape3DMaterial> t_material = _terrain->get_material(); - bool auto_enabled = t_material->get_auto_shader(); + if (t_material.is_null()) { + return Vector3(real_t(get_base(src)), real_t(get_overlay(src)), real_t(get_blend(src)) / 255.0f); + } + bool auto_enabled = t_material->get_auto_shader();🤖 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 `@modules/worldscape_3d/worldscape_3d_data.cpp` around lines 796 - 799, In the terrain material handling block, update the guard around _terrain and get_material() so t_material is validated before calling get_auto_shader(). Preserve the existing behavior when both references are valid, and skip the material-dependent logic when _terrain->get_material() returns a null Ref.modules/worldscape_3d/editor/worldscape_3d_editor.cpp-881-886 (1)
881-886: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a validity check before you dereference the region.
Line 883 calls
region->set_edited(false)with no validity check. Every other loop over region arrays in this file checksis_valid()first, for example line 581 and line 727. Keep the handling consistent so a null entry cannot crash the editor.🛡️ Proposed fix
for (int i = 0; i < _edited_regions.size(); i++) { Ref<WorldScape3DRegion> region = _edited_regions[i]; + if (region.is_null()) { + continue; + } region->set_edited(false); // Make duplicate for redo backup _edited_regions[i] = region->duplicate(true); }🤖 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 `@modules/worldscape_3d/editor/worldscape_3d_editor.cpp` around lines 881 - 886, In the loop over _edited_regions, validate each Ref<WorldScape3DRegion> with is_valid() before calling set_edited(false) or duplicating it. Skip invalid entries while preserving the existing handling for valid regions.modules/worldscape_3d/editor/ui/double_slider.cpp-91-103 (1)
91-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the
_position_x == 0sentinel.Line 94 uses
_position_x == 0to detect the first call. If the label's x position is legitimately0,_position_xstays0on every call and theelsebranch never runs, so the label position is never adjusted. The test also compares areal_tto0exactly.Use an explicit boolean flag instead.
🐛 Proposed fix
Add a member to
double_slider.h:real_t _position_x = 0.f; + bool _position_x_captured = false;Then update the check:
- if (_position_x == 0) { + if (!_position_x_captured) { _position_x = _label->get_position().x; + _position_x_captured = true; } else {🤖 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 `@modules/worldscape_3d/editor/ui/double_slider.cpp` around lines 91 - 103, Replace the `_position_x == 0` first-call sentinel in `DoubleSlider::update_label` with an explicit boolean member declared in `double_slider.h`. Initialize the flag as not yet captured, store the label’s initial x position only on the first update, then adjust the label position on subsequent calls regardless of whether that initial position is zero.modules/worldscape_3d/editor/worldscape_3d_editor.cpp-846-854 (1)
846-854: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winScale the movement average by the actual history size.
Line 854 always divides by 8. At the start of a stroke the history holds fewer than 8 entries, so
_operation_movementis scaled down proportionally._operation_movementdrivesdynamic_angleon line 408 and the gradient direction on line 348, so the first strokes use a direction vector of the wrong magnitude.Divide by the number of accumulated entries instead.
🐛 Proposed fix
// size -1, dont add the last appended entry for (int i = 0; i < _operation_movement_history.size() - 1; i++) { _operation_movement += _operation_movement_history[i]; } - _operation_movement *= 0.125f; // 1/8th + _operation_movement /= real_t(_operation_movement_history.size());🤖 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 `@modules/worldscape_3d/editor/worldscape_3d_editor.cpp` around lines 846 - 854, Update the averaging logic after the _operation_movement_history loop to scale _operation_movement by the actual number of accumulated history entries rather than the fixed 0.125f factor. Use the same count represented by the loop (excluding the last appended entry), while preserving the existing history accumulation behavior.modules/worldscape_3d/editor/ui/multi_picker.cpp-136-146 (1)
136-146: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a valid checked icon and make
init()idempotent.
get_editor_theme_icon()is inherited fromControl, andColorPickis a validEditorIconsname. However,picker_checkedhas no definition, so selected buttons receive a null icon. Use an existing icon or definepicker_checked.NOTIFICATION_POST_ENTER_TREEcan callinit()after each re-entry. Guard initialization using the created buttons or a dedicated flag. Do not use_points.is_empty(), becauseset_points()can populate_pointsbeforeinit()runs.🤖 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 `@modules/worldscape_3d/editor/ui/multi_picker.cpp` around lines 136 - 146, Update MultiPicker::init() to use a valid editor theme icon for _icon_picker_checked, or add the corresponding picker_checked icon definition. Make init() idempotent by returning when the button collection or dedicated initialization flag indicates buttons were already created; do not use _points.is_empty(), since set_points() may populate it before initialization.modules/worldscape_3d/editor/ui/worldscape_3d_asset_dock.cpp-623-626 (1)
623-626: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
_clearing_resourceis never assigned.The member is declared at
worldscape_3d_asset_dock.hLine 147 and initialized tofalse. No code path writes to it. This guard is therefore always false and has no effect. Remove it, or assign it around the clear operation if the guard was intended to suppress re-entrancy.🤖 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 `@modules/worldscape_3d/editor/ui/worldscape_3d_asset_dock.cpp` around lines 623 - 626, Update ListContainer::on_resource_changed and the resource-clearing flow to address the unused _clearing_resource guard: either remove the guard and its member when suppression is unnecessary, or set and reset _clearing_resource around the clear operation so the null-resource early return actually prevents re-entrant handling.modules/worldscape_3d/editor/ui/worldscape_3d_ui.cpp-381-387 (1)
381-387: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the no-op branch on the region map.
Line 385 assigns
r_map.get(index)back to the same index. The statement has no effect. The trailing// ?comment indicates the intent was not established during the port.Either delete the
elsebranch or implement the intended behaviour.♻️ Proposed cleanup
if (terrain->get_material()->get_world_background() == WorldScape3DMaterial::WorldBackground::NONE) { if (r_map.get(index) == 0 && _active_operation == WorldScape3DEditor::Operation::ADD) { r_map.set(index, -index - 1); - } else { - r_map.set(index, r_map.get(index)); // ? } }🤖 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 `@modules/worldscape_3d/editor/ui/worldscape_3d_ui.cpp` around lines 381 - 387, Remove the no-op else branch in the region-map handling within the terrain material background check, leaving the existing ADD operation that sets empty entries to -index - 1 unchanged. Do not retain the self-assignment or its trailing comment.modules/worldscape_3d/editor/ui/worldscape_3d_asset_dock.cpp-531-552 (1)
531-552: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe texture branch lacks the selection clamp that the mesh branch has.
Lines 549-551 clamp
_selected_idfor meshes after the rebuild. The texture branch has no equivalent. If textures are removed,_selected_idcan exceed the newtexture_count, and_brush_data["asset_id"]at Line 914 ofworldscape_3d_ui.cppthen carries an out-of-range texture ID into the paint operation.🛡️ Proposed fix
if (texture_count < WorldScape3DAssets::MAX_TEXTURES) { add_item(); } + if (_selected_id >= texture_count || _selected_id < 0) { + set_selected_id(0); + } } else {🤖 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 `@modules/worldscape_3d/editor/ui/worldscape_3d_asset_dock.cpp` around lines 531 - 552, Update the TYPE_TEXTURE branch in the asset rebuild logic to clamp _selected_id after repopulating textures, matching the mesh branch’s behavior: reset it to 0 when it is greater than or equal to texture_count or negative, while leaving valid selections unchanged.modules/worldscape_3d/editor/menu.cpp-52-60 (1)
52-60: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
TTRfor the menu item labels.Line 48 wraps the button text with
TTR. The popup item labels stay untranslated. Editor-facing strings must go throughTTRso they appear in the editor translation catalog.🌐 Proposed fix
PopupMenu *p = get_popup(); - p->add_item("Directory Setup...", MENU_DIRECTORY_SETUP); - p->add_item("Pack Textures...", MENU_PACK_TEXTURES); // TODO channel packer + p->add_item(TTR("Directory Setup..."), MENU_DIRECTORY_SETUP); + p->add_item(TTR("Pack Textures..."), MENU_PACK_TEXTURES); // TODO channel packer p->add_separator("", MENU_SEPARATOR); - p->add_item("Bake ArrayMesh...", MENU_BAKE_ARRAY_MESH); - p->add_item("Bake Occluder3D...", MENU_BAKE_OCCLUDER); + p->add_item(TTR("Bake ArrayMesh..."), MENU_BAKE_ARRAY_MESH); + p->add_item(TTR("Bake Occluder3D..."), MENU_BAKE_OCCLUDER); p->add_separator("", MENU_SEPARATOR2); - p->add_item("Set up Navigation...", MENU_SET_UP_NAVIGATION); - p->add_item("Bake NavMesh...", MENU_BAKE_NAV_MESH); + p->add_item(TTR("Set up Navigation..."), MENU_SET_UP_NAVIGATION); + p->add_item(TTR("Bake NavMesh..."), MENU_BAKE_NAV_MESH);🤖 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 `@modules/worldscape_3d/editor/menu.cpp` around lines 52 - 60, Update the popup menu item labels in the menu initialization code around get_popup() to wrap every editor-facing string passed to add_item, including “Directory Setup...”, “Pack Textures...”, both bake labels, and the navigation labels, with TTR; leave separators and menu IDs unchanged.modules/worldscape_3d/editor/menu/channel_packer.cpp-529-540 (1)
529-540: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
_vboxbypasses_margin, so the margin container has no effect.Line 535 adds
_marginto the dialog. Line 540 adds_vboxto the dialog as a sibling instead of a child of_margin. The only child_marginever receives is_save_file_dialogat Line 611. The margin constants at Lines 531-534 therefore never apply to the dialog content. Those constant names also carry the incorrecttheme_override_constants/prefix.🐛 Proposed fix
- _margin->add_theme_constant_override("theme_override_constants/margin_left", 5); - _margin->add_theme_constant_override("theme_override_constants/margin_top", 5); - _margin->add_theme_constant_override("theme_override_constants/margin_right", 5); - _margin->add_theme_constant_override("theme_override_constants/margin_bottom", 5); + _margin->add_theme_constant_override("margin_left", 5); + _margin->add_theme_constant_override("margin_top", 5); + _margin->add_theme_constant_override("margin_right", 5); + _margin->add_theme_constant_override("margin_bottom", 5); add_child(_margin); _vbox = memnew(VBoxContainer); _vbox->set_v_size_flags(Control::SIZE_EXPAND_FILL); _vbox->set_alignment(BoxContainer::ALIGNMENT_CENTER); - add_child(_vbox); + _margin->add_child(_vbox);🤖 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 `@modules/worldscape_3d/editor/menu/channel_packer.cpp` around lines 529 - 540, Make _vbox a child of _margin instead of adding it as a sibling to the dialog, so the dialog content is enclosed by the margin container. Update the four margin override keys in the initialization near _margin to use the correct theme-constant property names without the theme_override_constants/ prefix.modules/worldscape_3d/editor/menu/channel_packer.cpp-623-632 (1)
623-632: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the generated height image against a null
Refand tiny source images.Line 624 dereferences the return value of
WorldScape3DUtil::luminance_to_height()without a null check. Line 631 computesw / 4andh / 4with integer division. For an albedo smaller than 4 pixels in either dimension, the resize target becomes 0 andImage::resizefails.🛡️ Proposed fix
auto height_texture = WorldScape3DUtil::luminance_to_height(_albedo_panel->get_image()); - if (height_texture->is_empty()) { + if (height_texture.is_null() || height_texture->is_empty()) { EditorNode::get_singleton()->show_warning("Height Texture Generation error"); return; } // blur the image by resizing down and back... int w = height_texture->get_width(); int h = height_texture->get_height(); - height_texture->resize(w / 4, h / 4); + height_texture->resize(MAX(1, w / 4), MAX(1, h / 4)); height_texture->resize(w, h, Image::INTERPOLATE_CUBIC);🤖 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 `@modules/worldscape_3d/editor/menu/channel_packer.cpp` around lines 623 - 632, Update the height texture generation flow around WorldScape3DUtil::luminance_to_height to return with the existing warning when the returned Ref is null or empty before dereferencing it. Ensure the downscale dimensions in the subsequent resize calls are clamped to at least 1 pixel per axis so images smaller than 4 pixels remain valid.modules/worldscape_3d/editor/ui/worldscape_3d_tools.cpp-664-668 (1)
664-668: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid mutating the static unit strings in
init().String(const char *)treats each literal byte as Latin-1, sosubstr(1, 1)relies on UTF-8 encoding. On the nextNOTIFICATION_POST_ENTER_TREE, it runs on the one-character result and stores an empty string. UseString::chr(0x00B0)andString::chr(0x00B1)with non-static constants.🤖 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 `@modules/worldscape_3d/editor/ui/worldscape_3d_tools.cpp` around lines 664 - 668, Update the unit-string initialization in init() to use non-static constants constructed with String::chr(0x00B0) and String::chr(0x00B1). Remove the substr-based mutation so repeated NOTIFICATION_POST_ENTER_TREE calls preserve both degree and plus-minus symbols.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@modules/worldscape_3d/editor/worldscape_3d_editor.cpp`:
- Around line 1415-1417: Update the setting key checked in
setup_editor_settings() to match the initialized and read key
"terrain_editor/config/alt_key_bind", so an existing alternate-key preference is
preserved and only a missing setting is initialized to 0.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b6fe0ed8-9172-4701-be54-ef02b9429c5a
📒 Files selected for processing (3)
modules/worldscape_3d/editor/ui/worldscape_3d_asset_dock.cppmodules/worldscape_3d/editor/ui/worldscape_3d_tools.cppmodules/worldscape_3d/editor/worldscape_3d_editor.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/worldscape_3d/editor/ui/worldscape_3d_tools.cpp
- modules/worldscape_3d/editor/ui/worldscape_3d_asset_dock.cpp
There was a problem hiding this comment.
🔇 Additional comments (2)
modules/worldscape_3d/editor/gizmos.cpp (1)
63-65: Preserve the reset path for every null editor.The new guard prevents the dereference, but it returns without clearing
_show_rect,_region_size, or_gridwhenpositionis non-zero andeditoris null. The first condition does not run in that case, so previous gizmo geometry can remain visible. Move the!editorcheck into the existing reset branch or call the reset path before returning.modules/worldscape_3d/worldscape_3d_data.cpp (1)
161-162: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the complete 4096 region-size contract.
This check accepts
4096, butchange_region_size()later passes the value to_terrain->set_region_size((WorldScape3D::RegionSize)p_new_size)at Line 214. If the enum, setter validation, or resource loader still supports only64through2048, this function can remove the old regions and create4096-sized replacements before the setter rejects the value. Confirm that enum values, setters, load/save paths, editor hints, and documentation all accept4096.Verification script
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5407be99-f3da-404b-b7b4-25608430b413
📒 Files selected for processing (3)
modules/worldscape_3d/editor/gizmos.cppmodules/worldscape_3d/worldscape_3d.cppmodules/worldscape_3d/worldscape_3d_data.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/worldscape_3d/worldscape_3d.cpp
1 participant
Ported Terrain3D integration from ReX.
Now renamed as WorldScape3D.
Summary by CodeRabbit