Emit a signal when the height map is altered
Closed this issue · 9 comments
Description
I'm working on level design tools for my project. One useful function I'd like to have is the ability for props manually placed in the world to automatically move up/down when the terrain height map is edited. This would help avoid objects accidentally ending up floating, or buried underground.
What I would need from Terrain3D, to be able to implement this, would be a signal that is emitted when the height map changes, with an AABB of the area edited as a parameter. This could be on either the Terrain3D node itself, on the Terrain3DStorage. ProtonScatter integration could also benefit from this - to automatically trigger regeneration of scatter nodes.
Without the signal, the only options are to either poll the terrain periodically and update every prop in the scene frequently - slow in large scenes - or hack Terrain3D's plugin scripts and find a way to emit the signal that way.
Are you willing to help create this feature?
Yes
It's fine to issue signals. There are several update functions that could emit them when things are regenerated.
with an AABB of the area edited as a parameter
Everything gets regenerated on changes. We should adjust that to mark modified regions as dirty (See #165) so only those are allocated for undo/redo. I can see the signal issuing a list of modified regions. Issuing an AABB though would be only for your specific tool which seems like an odd design choice for us to do unless there's another way to justify its inclusion?
Everything gets regenerated on changes.
Everything gets regenerated? I probably wasn't very clear. This is what I'm doing:
Screencast.from.2023-12-11.19-01-49.webm
My script moves manually-placed nodes (the rocks) up/down when the heightmap is edited. I could do this in scenes with many times more nodes if my scripts could determine when an edit occurs, and the actual positions that were edited. The Terrain3DEditor class has the information I need - it only changes pixels within [-brush_size/2, brush_size/2] of the position clicked.
The AABB would be useful to editor scripts other than mine, including the project_on_terrain script in the Terrain3D repo provided for ProtonScatter integration. If you'd rather not have it though I'm happy to just keep this in my own fork.
Everything gets regenerated? I probably wasn't very clear. This is what I'm doing:
On every pass of the brush, force_update_maps() is called, which rebuilds all height regions if a height brush is used.
Screencast.from.2023-12-11.19-01-49.webm
That looks cool.
The Terrain3DEditor class has the information I need - it only changes pixels within [-brush_size/2, brush_size/2] of the position clicked.
Sure, so when it does force_update_maps, the editor could also emit a signal for a brush pass with the position. And it should already have a function for getting brush size data. Or I suppose it could emit an AABB. I'm not sure what is the most universally useful, AABB or center position.
IMHO, AABB is more general-purpose. Center position is only relevant if you're using a tool that relies on brushes. The add/remove region tool, or hypothetical tools like copy/paste/rotate/scale/fill selection, wouldn't have the same concept of brushes with position & size.
The signature of the signal could be something like: maps_edited(map_type: MapType, edited_area: AABB)
If we went ahead with this, would you prefer if
- The signal was on the Terrain3DEditor class, which is exposed to editor scripts by being a singleton, so that scripts can find and connect to the signal.
- Or if the signal is on Terrain3DStorage, and Terrain3DEditor calls a method on storage that emits it.
- Or is there a better way to expose the signal?
I think 1. Storage has no facility for partial updates, nor even region updates at the moment and only the editor knows what is being edited. We can move it around later if we find a better design.
Completed in #274
You know, now I'm thinking it seems strange to making the editor a singleton and have it issue signals.
Terrain3DStorage already these signals:
- height_maps_changed
- region_size_changed
- regions_changed
I think your second suggestion makes much more sense. The editor could update storage's edited AABB variable and initiate the signal send, just as it does to update_heights for the world AABB. So it already signaled on heightmap changes, but you wanted it to offer the AABB.
It makes more sense to me that 3rd party tools will link to the Terrain3D node, as will raycasts. From there it's easy and appropriate to connect to its storage signals. I could also see it connecting to the EditorPlugin. But to connect to the Terrain3DEditor?? It's just a fancy paint brush. What if the terrain storage is modified without using the editor?
No rush, but I think we should move consolidate the signal into storage.
What do you think?
My thinking with putting the signal on Terrain3DEditor is that it's only emitted when the editor changes something about the terrain, not if storage is altered directly by code. What you say makes sense though, and connecting to storage is probably more convenient for the user, especially if there are multiple terrains. I'll prep a new PR.
Yeah, I think the editor class should be dumb. Just a fancy brush. It doesn't even extend Node, it's just a raw object. I'd rather direct users to connect to storage, terrain3d.