-
Notifications
You must be signed in to change notification settings - Fork 148
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
Implement Geomorphing, Circular LODs, Symmetric grid #622
base: main
Are you sure you want to change the base?
Conversation
Does this resolve #191? |
It doesnt implement a veiwport size based adjustment to mesh size, but it does very much reduce how noticable transitions are between LODs since they smoothly interpolate now. |
3c4e804
to
c03e2ec
Compare
Switching to Full / Editor is 10x slower. I thought it was going to crash at first. It's an extraordinary amount of time for only 3 regions in the demo. Dynamic Editor at max size/radius is also quite slow to generate. The engine halts for a half second every update. Strangely CPU/GPU time don't report it, even with view gizmos turned off. Actually, even at normal size, dynamic editor is noticeably sluggish. Turn off gizmos. Then compare with Dynamic Game and move the camera top down, to the side. FPS is high, but the display noticeably stutters. No console messages. Haven't looked at the code yet.
|
Sculpting height is fixed for me after 1dc804f It seems the generate face loop in _get_shape_data takes an entire milisecond 😬 |
After some testing, and minor shuffling about, I can shave off a bit of time for a 65x65 size shapes from about 1.5ms to 1.2ms per _get_shape_data() the generate faces loop creates the faces needed for a trimesh (concave) collision shape, so is required as far as I can see. Testing on main, for a 65x65 shape size, it takes around 0.6ms. So whilst this is slower, its not the main culprit. maybe set_shape_data() for concave collision shapes is just terribly slow? Tho I am not sure why it would be so much slower than heightmapshape. |
Godot does a whole load of processing to the faces data, that is handled entirley different to the heightmapshape methods. It may be better to write a PR for Godot to enable alternating quad diagonals as an option directly in the heightmap shape. |
Yes, but that could take 6 months. We should copy the HeightMapShape class and make it our own SymmetricHeightMapShape3D and include it here in this PR. That's how I made Terrain3DMaterial. ShaderMaterial wasn't designed to be extended through godot-cpp, so I ripped all the code and built on top of it. |
height_map_shape_3d.cpp makes calls to the Physics server which is either godot_physics_server_3d.cpp or jolt_physics_server_3d.cpp Which then pass the data to the respective module internals. I was able to modify https://github.com/godotengine/godot/blob/394508d26dcf1b7a9362453f9009c07d969f1a7e/modules/godot_physics_3d/godot_shape_3d.cpp#L2061C1-L2081C2 That worked for the collision (with GodotPhysics), but I dont think gdextension can get that deep? |
Which function call is the one causing the slowdown? Is it set_faces? 🤔 I don't see much complexity there though.
The only calls I see are to shape_create(), which just memnews an object and returns an RID. Or set_shape_data, which sets data inside the shape. We can do both of those. I looked through the code a bit and thought about options like making our own heightmap shape using the custom shape type. Or that GodotHeightMapShap3D is a GodotoncaveShape3D. Perhaps we could do our own processing and setting the data directly in the physics server to avoid setting faces. Ultimately, however I think any shape we make needs to be a GodotShape3D and implement all of its virtual functions. And we don't have access to it via godot-cpp. So this option is out. We can start on the Godot PR process, but I think our immediate options are either LOD0 not symmetric and fast or symmetric and slow. It's not just that it's slow, but it makes the engine halt and stutter, which is a big problem. Maybe we can put it in a thread? |
LOD0 can be not symetric, its the best compromise at this point. |
Ok. That still allows us to geomorph and have circular lods? |
Collision is fast again now! I can say after looking a lot more into the inner workings, that heightmap shape is handled very differently than concave. I can probably get a PR together for the engine in due time, tho it would mean touching both godotphysics and joltphysics modules, and the scene shapes for the editor debug mesh generation. Ive actually got godot building so I can chip away at that - and potentially things like dx12 mipmaps 🤔 . Ultimatley I should be able to boil it down to a check box, and an extra field in the data dict "alternating_grid". Ready for a good review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Thanks.
Does this replace Mike Savage's clipmap code or is it still a close extension of it?
Do the mesh components still move at different speeds?
Its a replacement, It ended up closer to the Hoppes layout, tho instead of 1 L shape mesh, I have 4 long 2 unit wide strips that move in their axis pairs to 2 of 3 positions for their respective axis, enabling each LOD level to move at its own speed.
the extra pad between LODs was needed due to a minimum 2 unit size for any given mesh, required for the gemorph vertex shader shift. So whilst slightly more complicated, it removed the need for the degenerate seam meshes. I also used pre-calculated offsets (which Mike mentioned in his blog, but never implemented), which simplifies the snap logic a fair bit. |
src/terrain_3d_geomesh.cpp
Outdated
} | ||
|
||
void Terrain3DGeoMesh::_bind_methods() { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to expose anything? Otherwise, there's no reason for us to register this class, or have it show up in the docs.
Is there a reason for the user to use update, update_aabbs, snap? Maybe.
src/terrain_3d_geomesh.cpp
Outdated
snap(_terrain->get_snapped_position(), _terrain->get_vertex_spacing()); | ||
} | ||
|
||
void Terrain3DGeoMesh::destroy() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add in more comments and logging, where needed. Most functions like this can start with an INFO describing the function. Use DEBUG for incremental details if needed. Use EXTREME if it's the same as DEBUG but it's called every frame/operation/etc. And add some more comments if the logs aren't explanatory enough, eg _generate_offset_data() is sparse.
@@ -118,9 +121,16 @@ void Terrain3D::__physics_process(const double p_delta) { | |||
if (is_instance_valid(_camera_instance_id) && _camera->is_inside_tree()) { | |||
Vector3 cam_pos = _camera->get_global_position(); | |||
Vector2 cam_pos_2d = Vector2(cam_pos.x, cam_pos.z); | |||
RS->material_set_param(_material->get_material_rid(), "_camera_pos", cam_pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Terrain3D is going to insert the camera pos into the shader, why does it call update_maps() to insert mesh_size? Do we need to update the maps for the mesh size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also updates the shader's camera position at 60fps, whereas before it was potentially 300-600fps. So geomorphing and dual scaling cannot update faster than 60fps, while the real camera updates at 300-600fps.
In characters, there's an issue if the camera is following the player, and the camera and player move at different frame rates (_phys_proc and _proc). The character will vibrate. I haven't seen that yet, but this looks like it might be setup for that under certain conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_maps() already handled vertex_spacing, so it seemed suitable to add mesh_size update there as well.
The shader needs to know the mesh size for the geomorph blending (vertices are de-scaled, and then the vertex distance from the camera determine the blending value, with mesh_size being a mid-point. So that closer is not shifted at all, and further way is shifted fully.) world_space is used to determine odd/even for each vertices shift direction only.
If the camera travel speed is far in excess of the snapping, then updating the position at true FPS would break the blend, and lead to seems showing up. so it should actually happen at the same rate as the actual vertices (un-shifted) move.
there is no reason using the camera matrix value as for before for dual scaling couldn't be the case.
src/terrain_3d.cpp
Outdated
ADD_PROPERTY(PropertyInfo(Variant::INT, "mesh_size", PROPERTY_HINT_RANGE, "8,64,1"), "set_mesh_size", "get_mesh_size"); | ||
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "vertex_spacing", PROPERTY_HINT_RANGE, "0.25,10.0,0.05,or_greater"), "set_vertex_spacing", "get_vertex_spacing"); | ||
ADD_PROPERTY(PropertyInfo(Variant::INT, "mesh_size", PROPERTY_HINT_RANGE, "16,64,2"), "set_mesh_size", "get_mesh_size"); | ||
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "vertex_spacing", PROPERTY_HINT_RANGE, "0.25,10.0,0.01,or_greater"), "set_vertex_spacing", "get_vertex_spacing"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did these two change?
For vertex_spacing, as it is here, it doesn't show more fine grained than 0.05 in the inspector, but this might call it 5x as much when sliding in the inspector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see an issue, but Grok seems to think _mesh_size should be limited to a power of 2.
How would _mesh_size or LOD levels need to change to cause test_x or test_z to be out of range or misplace edge strips?
In Terrain3DGeoMesh::snap(), the variables test_x and test_z are calculated as:int test_x = CLAMP(int(round((pos.x - next_x) / snap_step)) + 1, 0, 2);
int test_z = CLAMP(int(round((pos.z - next_z) / snap_step)) + 1, 0, 2);
These determine edge strip placements by comparing the current LOD’s snap position (pos) to the next LOD’s snap position (next_x, next_z) relative to snap_step. The CLAMP ensures values stay between 0 and 2, but changes to _mesh_size or LOD levels could still lead to miscalculations or misplacements:
Non-Power-of-Two _mesh_size:
The clipmap system assumes _mesh_size (e.g., 16, 32, 64) is a power of two for even division into snap steps. If _mesh_size = 24 (not a power of two), snap_step might not align with the mesh grid, causing (pos.x - next_x) / snap_step to produce values that, before clamping, are outside 0 to 2. After clamping, this could place edge strips incorrectly, leading to gaps or overlaps.
Very Small _mesh_size (e.g., 2 or 4):
A tiny mesh size reduces the number of tiles, potentially making snap_step too large relative to the mesh. This could cause test_x and test_z to frequently clamp to 0 or 2, misplacing edge strips and leaving parts of the terrain uncovered or misaligned with higher LODs.
Very Large _mesh_size (e.g., 1024):
An excessively large mesh might introduce floating-point precision errors in pos.x - next_x, though this is less common. More critically, if snap_step doesn’t scale proportionally, edge strips might not span the intended areas, causing visual discontinuities.
Increased LOD Levels:
Adding more LOD levels beyond the current design (e.g., exceeding precomputed offsets in _generate_offset_data()) could misalign next_x and next_z. If the clipmap layout or offset data doesn’t account for these extra levels, test_x and test_z might clamp correctly but point to incorrect edge strip positions for higher LODs, disrupting the terrain’s continuity.
Clipmap Layout Changes:
If the tile, trim, or edge arrangement changes (e.g., different spacing or additional mesh types), the assumptions in snap() might not hold. For example, a denser clipmap could shift next_x unexpectedly, pushing test_x outside the expected range before clamping, resulting in misplaced strips.
Example: With _mesh_size = 16 and snap_step = 32 * _vertex_spacing, if _mesh_size changes to 20, the uneven division might make (pos.x - next_x) / snap_step yield a value like -0.8 or 2.3 before the +1 and clamp, leading to clamped values that don’t match the intended edge strip positions. This could create gaps or overlaps in the terrain.
Mitigation: Keep _mesh_size as a power of two and dynamically adjust offset data based on _mesh_size and LOD levels to ensure accurate snap calculations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I increased mesh size limit as less than 16 was breaking things, even with the original clipmap, gaps between LODs would appear.
vertex spacing being more granular than before was a result of me making sure the clipmap movement was actually robust, I noticed before some tiny gaps when some values were set, fixed now. (missing rounds, either in snap(), or in the shader)
It can be increased back to 0.05, tho for users with imported real world data, maybe a more specific scaling would be appreciated?
As for Grok, precision errors would accumulate without the round, where some values might be 0.9999 instead of 1.0, and then set the wrong index offset as the int cast would floor that to 0. but round() is there to catch that.
test_x and test_z determine the current LOD position reletive to the next LOD on that axis, and return either "nearside-edge, in the middle, or offside-edge" any float precision error would need to be extreme to cause problems, that 0.9999 would have to be 0.4999 to cause an incorrect positioning.
closes #158
closes #209
closes #383
partially addresses #421
closes #553
Clipmap method re-written. now exclusivly built from rectangular meshes, with a minimum 2 unit width/length.
renamed geoclipmap to Terrain3DGeoMesh class object, and moved related functions from Terrain3D into this class.
main shader, and extra shaders updated to implement geomorphing.
Geomorph is required as the clipmap does not have any seam meshes.
Minimum mesh size raised to 16 (things break down less than that, even with the old clipmap)
Vertex spacing minimum stepping adjusted to 1/16th (0.0625) prevents a few artifacts with the new methods.
v_camera_pos has been replaced by a uniform _camera_pos, and all relevent shader inserts updated.
Required as the shadow pass would set VIEW_MATRIX[3] to the light source position, resulting in a missmatch between the vertex positions between shadow/standard render passes
The entire mesh is now an alternating grid.
Still to update collision to match (should hopefully be fairly straight forwards)Edit: Collision grid now matches.
Current issues:
painting tools dont work untill swapping away / back to the scene on first load.pretty sure ive not correctly implemented multiple things..some crashes when closing / reloading.Im not very familiar with c++ in general and have stumbled my way through this so far.. its "working" but Im a bit in the dark in regards to polishing this up.. an early review would be greatly appreciated!
Godot_v4.3-stable_win64_ayU9jwM6PB.mp4