feat: Instancer LOD #604

laurentsenta · 2025-01-25T16:44:09Z

Contributes to #43
Introduce four levels of LODs for multimesh instances and min / max shadow lods.

See thread in Discord

Task list:

Regarding validation:

This PR introduces a bunch of fields that are related to each other:

maximum lod value is valid if the scene is loaded and must be less than mesh count
maximum shadow lod is valid if <= maximum lod and >= minimum shadow lod (and vice versa for minimum shadow lod)
Cast Shadows validity depends on the shadow lods:
- if cast shadow is double-sided or shadows only, it will interact weirdly with min/max shadow lods

Checking values in the setters will be super annoying to use, as in "I'm trying to lower the maximum lod, but nothing happens" > "yes, you need to lower the minimum shadow lod first".

This is a perfect use case for configuration warning, which is a work in progress:

src/terrain_3d_mesh_asset.h

src/terrain_3d_instancer.cpp

src/terrain_3d_mesh_asset.h

laurentsenta · 2025-01-25T18:54:30Z

src/terrain_3d_mesh_asset.cpp

+	_shadow_lod = CLAMP(p_lod, 0, get_maximum_lod());
+	LOG(INFO, "Setting shadow LOD: ", _shadow_lod);
+	emit_signal("instancer_setting_changed");
+}


Need maintainer input: every time I reopen the project, the maximum lod and shadow lod values are reset to -1,
I think this is because the mesh is not loaded when the editor tries to set the instance's value.

If you change the values and then close and reopen the scene, that'll work.

Is there an idiomatic way to avoid this?

When Godot loads the scene file it loads all settings from the scene file. You don't really know which one it will load first. If this is loaded before the scene, get_mesh_count() will be zero, which sets your _maximum_lod = -1.

If packed_scene.is_null() then you shouldn't check get_mesh_count() and accept the value as it is. It won't be used while the scene is null.

When packed_scene is loaded, you can verify that maximum_lod is valid. Same w/ shadow lod.

Done in fcaccb9

Xtarsia

Very nice work overall.

Cory will be along asking for a good squash no doubt..

The demo changes can likley be dropped unless you want to spend time tidying them up.

Docs effort is stellar!

src/terrain_3d_instancer.cpp

TokisanGames · 2025-01-27T16:55:32Z

Cory will be along asking for a good squash no doubt..

It can be squashed down to 3 commits: code, docs, demo changes.

I'll review in a couple of days after Xtarsia is satisfied and I get more progress on my collision commit.

Saul2022 · 2025-01-28T10:54:37Z

Looking great, just one curiosity, so to add lods, we do it like we usually do with visibility ranges and hlod.

TokisanGames · 2025-01-28T15:13:55Z

@Saul2022 Should be able to setup lods in your mesh scene like this, and that's it. Or adjust defaults if desired.

TokisanGames

Alright, very good work on this. Thank you for the effort. Please rebase it as you address the notes.

Playing with the options and thinking about the names, I think they can use some adjustments to be clearer. I'd prefer to move away from "higher, lower, minimum, maximum" because lods numbers are a reverse scale, where higher/better is a lower number. The docs say "lods above this...", does "above" mean "larger number / lower detail"? Or "higher detail / lower number"? Does "maximum lod" mean "maximum distance / lowest detail" or "maximum detail / lowest distance"? I think "last" is not only shorter, but more universal in meaning lowest detail, farthest away.
- maximum_lod means "don't show lods lower than this lod". I think last_lod is a little better. It's also lod_count-1.
- maximum_shadow_lod means "show shadows for this lod and higher(0) - ie don't show shadows for lods lower than this value". This would be better named last_shadow_lod.
- minimum_shadow_lod means "use this shadow impostor lod or lower(3) - ie when showing a lod higher(0) than this lod, use this one as its shadow impostor". This would be better named max_shadow_imposter_lod, or max_shadow_lod.
If minimum_shadow_lod is > maximum_shadow_lod, the latter doesn't work. Or more clearly, if max_shadow_imposter_lod > last_shadow_lod, then it shows shadows beyond the last_shadow_lod. Given the above descriptions it should not. It should only show shadows for lods up to last_shadow_lod.
I actually don't think we need a maximum_shadow_lod. Lights already have a shadow distance. We don't need to cull it ourselves. Having it prevents us from needing to create and store the MMI, but that generation is quick and only on start, and the memory should be shared with my related note elsewhere.

In my specifications here on discord I expressed the need for maximum_lod(last_lod) and shadow_lod. The lods should show shadows at all distances that the user has configured their Lights for, it just may be at a lower level LOD.

MeshAsset/Cast Shadows / shadows only doesn't work. Shadows Off does work.
MeshAsset set_scene_file doesn't do any filtering or sorting on meshes named LOD0-LOD3. It happens that sometimes the LODS might be out of order for an artist or an engine reason. I would look for all of the mesh instances that end in LOD#, then sort them and use the first 4. If I couldn't find any LODs, then just take the first original 4. Maybe print some messages saying, "found 8 lods, using the first 4." or "Meshes don't end with LOD#, assuming first 4 are LOD0-LOD3".
MeshAsset set_scene_file - You could also fix a bug where if they provide a scene with a MeshInstance3D as the root node, it won't use it. Probably because the find_child starts looking at the children and ignores the scene root.

TokisanGames · 2025-01-31T18:09:24Z

src/terrain_3d_mesh_asset.cpp

+	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "lod_1_visibility_range", PROPERTY_HINT_RANGE, "0.,4096.0,.05,or_greater"), "set_lod_1_visibility_range", "get_lod_1_visibility_range");
+	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "lod_2_visibility_range", PROPERTY_HINT_RANGE, "0.,4096.0,.05,or_greater"), "set_lod_2_visibility_range", "get_lod_2_visibility_range");
+	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "lod_3_visibility_range", PROPERTY_HINT_RANGE, "0.,4096.0,.05,or_greater"), "set_lod_3_visibility_range", "get_lod_3_visibility_range");
+	ADD_PROPERTY(PropertyInfo(Variant::INT, "mesh_count", PROPERTY_HINT_NONE), "set_mesh_count", "get_mesh_count");


You can specify "" for the setter, then you don't need set_mesh_count or _validate_property.

TokisanGames · 2025-01-31T18:11:44Z

src/terrain_3d_mesh_asset.cpp

+
+//DEPRECATED 1.0 - Remove 1.1
+void Terrain3DMeshAsset::set_visibility_range(const real_t p_visibility_range) {
+	set_lod_0_visibility_range(p_visibility_range);


visibility_range marks the distance the plants are visible at all, not necessarily lod0. I would have this set the distance value of the last lod, as that will be the closest to previous behavior.

TokisanGames · 2025-01-31T18:34:46Z

src/terrain_3d_mesh_asset.cpp

+	}
+
+	return MIN(MAX_LOD_COUNT - 1, get_mesh_count() - 1);
+}


I'm confused by this function and the handling of maximum_lod (last_lod).

lod_count is CLAMP(_packed_scene.is_valid() ? _meshes.size() : 1, 1, MAX_LOD_COUNT)
and last_lod is lod_count - 1.

If there is no packed scene, there's still 1 generated mesh. The defaults can be appropriate for when there's only 1 mesh (which may be generated or not).

TokisanGames · 2025-01-31T18:36:25Z

src/terrain_3d_instancer.h

@@ -47,8 +47,9 @@ class Terrain3DInstancer : public Object {
 	void _destroy_mmi_by_location(const Vector2i &p_region_loc, const int p_mesh_id);
 	void _backup_regionl(const Vector2i &p_region_loc);
 	void _backup_region(const Ref<Terrain3DRegion> &p_region);
-	Ref<MultiMesh> _create_multimesh(const int p_mesh_id, const TypedArray<Transform3D> &p_xforms = TypedArray<Transform3D>(), const PackedColorArray &p_colors = PackedColorArray()) const;
+	Ref<MultiMesh> _create_multimesh(const int p_mesh_id, const int lod, const TypedArray<Transform3D> &p_xforms = TypedArray<Transform3D>(), const PackedColorArray &p_colors = PackedColorArray()) const;


lod should be p_lod

TokisanGames · 2025-01-31T18:40:41Z

src/terrain_3d_mesh_asset.h

@@ -88,6 +121,7 @@ class Terrain3DMeshAsset : public Terrain3DAssetResource {

 	Ref<Mesh> get_mesh(const int p_index = 0);


index can be renamed to lod

TokisanGames · 2025-02-01T10:14:07Z

src/terrain_3d_mesh_asset.cpp

+		return get_cast_shadows();
+	}
+	// That lod relies on the min shadow lod
+	if (p_lod_id <= get_minimum_shadow_lod()) {


// This lod uses the shadow impostor lod

TokisanGames · 2025-02-01T10:23:42Z

src/terrain_3d_instancer.cpp

 			}
 		}
 	}
 }

+void Terrain3DInstancer::_setup_mmi_lod(MultiMeshInstance3D *p_mmi, const Ref<Terrain3DMeshAsset> &p_ma, const int p_lod) {


This should be named _setup_mmi_lod_ranges

TokisanGames · 2025-02-01T10:29:27Z

src/terrain_3d_instancer.cpp

+
+					// Create MM and assign to MMI
+					mmi = cell_mmi_dict[cell];
+					mmi->set_multimesh(_create_multimesh(mesh_id, lod, xforms, colors));


This looks like you are creating 5 MultiMeshes for Shadow, and LOD0-3. So that's 5 meshes (necessary) and 5x the transforms and colors (MMI internals copy incoming data so we can't share it) to display 4 meshes.

You could instead reverse the for loop to count from last_lod to SHADOW_LOD. Then when creating the shadow MMI, grab the already created LOD# Multimesh reference and insert it instead of creating an identical copy.

TokisanGames · 2025-02-01T10:35:02Z

src/terrain_3d_mesh_asset.cpp

 	ClassDB::bind_method(D_METHOD("get_thumbnail"), &Terrain3DMeshAsset::get_thumbnail);

 	ADD_PROPERTY(PropertyInfo(Variant::STRING, "name", PROPERTY_HINT_NONE), "set_name", "get_name");
 	ADD_PROPERTY(PropertyInfo(Variant::INT, "id", PROPERTY_HINT_NONE), "set_id", "get_id");
 	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "height_offset", PROPERTY_HINT_RANGE, "-20.0,20.0,.005"), "set_height_offset", "get_height_offset");
 	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "density", PROPERTY_HINT_RANGE, ".01,10.0,.005"), "set_density", "get_density");
-	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "visibility_range", PROPERTY_HINT_RANGE, "0.,4096.0,.05,or_greater"), "set_visibility_range", "get_visibility_range");
-	//ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "visibility_margin", PROPERTY_HINT_RANGE, "0.,4096.0,.05,or_greater"), "set_visibility_margin", "get_visibility_margin");
+	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "visibility_margin", PROPERTY_HINT_RANGE, "0.,4096.0,.05,or_greater"), "set_visibility_margin", "get_visibility_margin");


Sort all of the lod settings under the scene file, and put this above those properties to put them in a group ADD_GROUP("LODs", "");. I don't know, but you might be able to then add ADD_GROUP("", ""); to get out of the group.

TokisanGames · 2025-02-01T10:39:51Z

src/terrain_3d_mesh_asset.cpp

+	return MIN(MAX_LOD_COUNT - 1, get_mesh_count() - 1);
+}
+
+void Terrain3DMeshAsset::refresh_valid_lods() {


This function should be called _validate_lods()

Use internal names, not functions.

laurentsenta marked this pull request as draft January 25, 2025 16:44

laurentsenta commented Jan 25, 2025

View reviewed changes

src/terrain_3d_mesh_asset.h Outdated Show resolved Hide resolved

laurentsenta commented Jan 25, 2025

View reviewed changes

src/terrain_3d_instancer.cpp Show resolved Hide resolved

laurentsenta force-pushed the feat/instancer-lod branch 4 times, most recently from 43ff44e to 0cc19cf Compare January 25, 2025 18:38

laurentsenta commented Jan 25, 2025

View reviewed changes

src/terrain_3d_mesh_asset.h Outdated Show resolved Hide resolved

laurentsenta commented Jan 25, 2025

View reviewed changes

laurentsenta force-pushed the feat/instancer-lod branch 3 times, most recently from ccdc7ab to 1f2af27 Compare January 26, 2025 09:06

laurentsenta changed the title ~~(wip) feat: Instancer LOD~~ feat: Instancer LOD Jan 26, 2025

laurentsenta marked this pull request as ready for review January 26, 2025 22:25

TokisanGames mentioned this pull request Jan 27, 2025

[Tracker] Improve foliage instancing #43

Open

20 tasks

Xtarsia reviewed Jan 27, 2025

View reviewed changes

src/terrain_3d_instancer.cpp Outdated Show resolved Hide resolved

src/terrain_3d_instancer.cpp Show resolved Hide resolved

laurentsenta added 3 commits January 29, 2025 17:50

feat: introduce LOD in instancer

6441bf4

demo: update demo with instancer LOD

7f67809

doc: add LOD instancer documentation

b6dff47

laurentsenta force-pushed the feat/instancer-lod branch from e2df2ff to b6dff47 Compare January 29, 2025 16:53

TokisanGames requested changes Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Instancer LOD #604

feat: Instancer LOD #604

laurentsenta commented Jan 25, 2025 •

edited

Loading

laurentsenta Jan 25, 2025

TokisanGames Jan 25, 2025

laurentsenta Jan 25, 2025

Xtarsia left a comment

TokisanGames commented Jan 27, 2025

Saul2022 commented Jan 28, 2025

TokisanGames commented Jan 28, 2025

TokisanGames left a comment •

edited

Loading

TokisanGames Jan 31, 2025

TokisanGames Jan 31, 2025

TokisanGames Jan 31, 2025

TokisanGames Jan 31, 2025

TokisanGames Jan 31, 2025

TokisanGames Feb 1, 2025

TokisanGames Feb 1, 2025

TokisanGames Feb 1, 2025

TokisanGames Feb 1, 2025

TokisanGames Feb 1, 2025

		@@ -88,6 +121,7 @@ class Terrain3DMeshAsset : public Terrain3DAssetResource {

		Ref<Mesh> get_mesh(const int p_index = 0);

feat: Instancer LOD #604

Are you sure you want to change the base?

feat: Instancer LOD #604

Conversation

laurentsenta commented Jan 25, 2025 • edited Loading

Regarding validation:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xtarsia left a comment

Choose a reason for hiding this comment

TokisanGames commented Jan 27, 2025

Saul2022 commented Jan 28, 2025

TokisanGames commented Jan 28, 2025

TokisanGames left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsenta commented Jan 25, 2025 •

edited

Loading

TokisanGames left a comment •

edited

Loading