Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maps_edited signal #325

Merged
merged 3 commits into from
Apr 17, 2024
Merged

Conversation

tcoxon
Copy link
Contributor

@tcoxon tcoxon commented Feb 19, 2024

First commit fixes the scale of the AABB parameter on the storage maps_edited signal. Due to a mistake it was getting scaled by mesh_vertex_spacing twice. add_edited_area is called with the AABB already in global coordinates, so there's no need to scale it again in get_edited_area.

Second commit adds the terrain_prop_group.gd script which makes use of this signal, and may also be of utility to users of Terrain3D. This script only runs in the editor. Its purpose is to move its children vertically up/down whenever they're moved or the terrain is edited, to try to preserve their vertical heights above the surface of the terrain. If a child is directly touching the terrain for instance, it will always snap to the surface of the terrain. This is very useful when modifying the heightmap of terrain in a scene you've already manually added props to!

To set up TerrainPropGroup, add it to the scene like this, with some models as children:

image

Then the nodes will 'snap' whenever moved, and automatically adjust up/down with the height of the terrain:

Screencast.from.2024-02-19.18-12-09.webm

@tcoxon tcoxon changed the title Maps edited signal maps_edited signal Feb 19, 2024
helper.name = CHILD_HELPER_NAME
p_node.add_child(helper, true, INTERNAL_MODE_BACK)
helper.transform_changed.connect(_on_child_transform_changed.bind(p_node))
assert(p_node.has_node(CHILD_HELPER_PATH))
Copy link
Owner

@TokisanGames TokisanGames Mar 10, 2024

Choose a reason for hiding this comment

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

What will the user do with the two asserts in this function on failure? How about pushing an error with instructions and aborting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These asserts are meant to check assumptions that the code makes. They're not user errors, but rather guards against bugs being introduced in future. Have you found a way to trigger assert failure?

return terrain


func _on_child_entered_tree(p_node: Node) -> void:
Copy link
Owner

Choose a reason for hiding this comment

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

If a child node is duplicated, this function is called, however execution does not make it to the end. So duplicated objects are not affected unless they are moved out of the group and back in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, great catch there. I will definitely fix that.

Copy link
Owner

@TokisanGames TokisanGames left a comment

Choose a reason for hiding this comment

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

Thanks for this. It looks very useful.

  • transform_changed_signaller -> transform_changed_notifier is consistent with Godot's use of notify and notifier in the rendering server, VisibleOnScreenNotifier3D, and various functions and variables.
    • Search help for signaller vs notif / notifier.
    • There are some signaling (note one L) names for use with WebRTC, but that's an entirely different use for the word signal.

Nodes

  • Moving a child node out of the Terrain3D adjacent TerrainPropGroup produced this error:
    Node not found: "../Terrain3D" (relative to "/root/@EditorNode@17141/@Panel@13/@VBoxContainer@14/@HSplitContainer@17/@HSplitContainer@25/@HSplitContainer@33/@VBoxContainer@34/@VSplitContainer@36/@VSplitContainer@62/@VBoxContainer@63/@PanelContainer@110/MainScreen/@CanvasItemEditor@9462/@VSplitContainer@9281/@HSplitContainer@9283/@HSplitContainer@9285/@Control@9286/@SubViewportContainer@9287/@SubViewport@9288/Demo/TerrainPropGroup/MeshInstance3D5").

Positions

  • Changing the position of an object in the inspector does not update the height, only with the gizmos.

  • Moving a tracked object (that has been moved before) out of defined regions, might result in a nan position and eventually crash. Or,

    • If an object hasn't moved yet, it may allow you to move it out fine. Then if you move it back in to defined regions, the object no longer tracks the terrain.
  • If a tracked object is on a region, then that region is removed, or

    • If a tracked object is on a void region and that region is added,
    • the position of that object is nan, the editor slows, and the console starts dumping errors:
      ERROR: Condition "!v.is_finite()" is true. at: instance_set_transform (servers/rendering/renderer_scene_cull.cpp:922)

@tcoxon
Copy link
Contributor Author

tcoxon commented Mar 15, 2024

Moving a child node out of the Terrain3D adjacent TerrainPropGroup produced this error:

This is Godot issue #80589. It happens whenever you reparent a MeshInstance3D node with the skeleton property set.

Changing the position of an object in the inspector does not update the height, only with the gizmos.

Not sure what you mean here. I've edited the X, Y and Z coordinates in the inspector and the height updates correctly.

Moving a tracked object (that has been moved before) out of defined regions, might result in a nan position and eventually crash. Or,
If a tracked object is on a region, then that region is removed, or

Thanks! I'll fix this in the script by treating NaNs as 0 height, so that you can still move nodes outside of regions.

@TokisanGames
Copy link
Owner

TokisanGames commented Mar 23, 2024

Right now edited_area is emitted for all tools. So all objects positions are adjusted for texture/color painting, and soon foliage. Maybe the signal should send maptype and operate only on heightmap.

Also still open is the decision on grouping affected nodes under a Node3D parent, or using a Godot group(tm).

tcoxon and others added 3 commits April 17, 2024 20:46
Moves its child nodes up/down whenever the terrain changes, or it's moved, to try to preserve nodes' vertical height above the terrain surface.
@TokisanGames
Copy link
Owner

As discussed on discord regarding updating children of a Terrain3DObjects node vs a Group(tm), the former is better because objects can be filtered by location. If there are a million objects, the AABB of the Terrain3DObjects nodes allows us to cut out huge amounts of objects that are not in the edited area. But notifying a group means all objects everywhere get and must process the signal. Though not implemented yet, positional filtering could be done.

The updates to this PR work well.

I still think there's another optimization improvement available, by emitting the map type so objects are not moved when non-sculpting operations occur.

Nice work as always, @tcoxon . Thanks!

@TokisanGames TokisanGames merged commit ea9eb16 into TokisanGames:main Apr 17, 2024
@tcoxon tcoxon deleted the maps_edited_signal branch July 12, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants