Support custom color & data for GridMap cells

[Mar0Lard]

Fixes godotengine/godot-proposals#8723
Added a color & custom data property on cells in Gridmaps, and assorted methods.
Custom colors and data can now be set on cells through the set_cell_item method.
Passed color/data can then be accessed in the item's spatial shader, allowing for rendering effects on specific cell items.

[smix8]

Hi, did you close this intentional?
You can turn your PR back to a Draft if you dont want it reviewed just yet if you need more time to rework stuff and then force push changes.

[Mar0Lard]

Thank you, I'll look into doing that; some tests failed so want to investigate why. Apologies, first time doing a pull request on GitHub so there's a little bit of trial and error involved!

[smix8]

While this is still a Draft some pointers to avoid work that might need to be changed later.

Considering custom_color and custom_data are niche that only a fraction of GridMap using projects will ever use this shouldn't be part of the main set_cell_item() function or the main Cell union struct.

Especially since the Cell tries to be as lean as possible by tightly packing its item/rot/layer properties. That is also why all the other more optional meta data is stored in other lookups like HashMaps and not in the main Cell.

That function and struct change also breaks function sig and compatibility unnecessarily (the reason for the current checks failing) and while solvable with compatibility functions this should be avoided.

So imo custom_color and custom_data should have their own setters and getters and own lookup, e.g. a HashMap to look up by cell / indexkey so that projects that do not use any of those new properties are mostly unaffected.

We may not even need the 2 bool properties use_custom_colors and use_custom_data to manually toggle by the user and the related errors. Can just check if anything uses one of the two custom properties to auto it.

Same with the error for having the bool properties not set, I dont think it is needed and might turn annoying quickly for users. Could add a has_custom_color() and has_custom_data() functions so things can check if a cell has anything. You cant just return "empty" color with the getter when a cell has nothing so you need such a function or other way to distinguish a default Color() from an intentional custom Color with the same value.

The multimesh should also only call its custom setters when things are actually used as you might have multimeshes and octants with custom value and others without, no need to allocate memory everywhere for something that will not be used.

[AThousandShips]

For the validation you also need to handle the compatibility breakgages, see here

[Mar0Lard]

Thank you very much for the in-depth comments! This all makes sense, will look at adressing all the feedback.

[Mar0Lard]

I remade the feature entirely following the advice above, this should now be ready for review! Thank you so much @smix8 for the detailed comment, it guided me towards a much better integration. I don't normally do C++ so this was an excellent challenge, I learned a ton untangling the code and will gladly contribute again where I can! Thanks again, looking forward to the review comments.

[smix8]

Still in favor of exposing that users can set the custom_color and custom_data on the GridMap internal MultiMeshes.

Although I am not certain with the current function naming here. I think there needs to be a prefix to clearly distinct the API between what is just generic GridMap cell data properties and what is very specifically targeting the MultiMesh custom_color and custom_data. That should also make it clear from a documentation point why there are very constrained Color parameters instead of e.g. a more flexible Dictionary. I think I have also seen a part that mixes between Color and Variant that I can not find it right now.

I fear that without a prefix by using the shorter function names we paint the API into a corner by having the shorter e.g. "set_cell_data()" already taken over by something very specific to multimesh.

[badsectoracula]

I was testing this PR - as well as #69646 which i found before noticing this PR and i think this is better as it also covers colors - and while i think it is very useful for a variety of uses (actually it doesn't have to be just for multimesh data since the scripts can access the stored values they can use them for any other functionality they may want, like setting up enemy spawn probability for a dungeon crawler for example - though more on that later) there are a couple of issues i found.

The first is that if the GridMap has baked meshes (this seems to be more of a hack since i can't find any use for this in the engine so it is probably meant to be called by an editor script, but i might be wrong here) they block any changes as they override any regenration. This can be fixed by adding:

if (baked_meshes.size() && !recreating_octants) {
	clear_baked_meshes();
	_recreate_octant_data();
}

...at the top of the set_cell_xxx calls to clear the mesh data (the other PR did that too so i assume it is something some people will need).

The second is probably more of a multimesh bug than a gridmap bug but the multimesh behaves like that for some time now (i checked Godot 4.0.0 and 4.0.4 and the behavior was still there) so it might be a be a backwards compatibility breakage if it changes.

The bug occurs with the gridmap when you set a color on a cell and you use the COLOR constant in a shader: all other cells in the octant get a black color:

The shader used above just multiplies the final color with COLOR. A script sets a random cell to a reddish color when some key is presset (in the shot the cell at bottom right is the one affected).

As i wrote the real issue here seems to be due to how multimesh handles custom colors: if a shader uses COLOR it will get a (1,1,1,1) in the shader if the multimesh does not use colors but if it uses colors the array where they are stored is initialized to (0,0,0,0). The end result is that in the gridmap, in the octants with meshes that use a shader with COLOR will get a black/zeres value for all cells that do not have a color set if there is at least one cell that sets a custom color while the meshes in octants that do not have any color will get a white/ones value instead.

This can happen with custom data too because the renderer seems to encode color and custom data as 16bit half-floats passed together into a single 32bit float vector, so even if a cell does not have a color set but there is a cell in the same octant with color or data, the cell will receive black/zero via COLOR.

There are two potential fixes for this. The first is to make sure that gridmap sets the color to white/ones (the otherwise default value for COLOR if none is provided) when either colors or data exists, e.g. this is how i modified my copy of the PR:

// in bool GridMap::_octant_update(const OctantKey &p_key) {

RID mm = RS::get_singleton()->multimesh_create();
RS::get_singleton()->multimesh_allocate_data(mm, E.value.size(), RS::MULTIMESH_TRANSFORM_3D, use_colors | use_data, use_data);
RS::get_singleton()->multimesh_set_mesh(mm, mesh_library->get_item_mesh(E.key)->get_rid());

int idx = 0;
for (const Pair<Transform3D, IndexKey> &F : E.value) {
	RS::get_singleton()->multimesh_instance_set_transform(mm, idx, F.first);

	if (use_colors) {
		Color col(1, 1, 1, 1);
		if (color_map.has(F.second)) {
			col = color_map[F.second];
		}
		RS::get_singleton()->multimesh_instance_set_color(mm, idx, col);
	} else if (use_data) {
		RS::get_singleton()->multimesh_instance_set_color(mm, idx, Color(1, 1, 1, 1));
	}

	if (data_map.has(F.second)) {
		Color dat = data_map[F.second];
		RS::get_singleton()->multimesh_instance_set_custom_data(mm, idx, dat);
	}

// more code follows

(notice the use_colors | use_data when calling multimesh_allocate_data)

The other solution is to change the behavior of multimesh to be consistent in what value is passed via COLOR to shaders regardless of if there are custom colors in the multimesh or not (preferably passing white/ones if none has set as that makes more sense for a color but could be either way). However my main concern is the current behavior has been around for some time (i tested its behavior with 4.0-stable so it has been around at least since the beginning of the current major version of Godot - perhaps more, but i had to manually apply parts of the PR and fix compilation bugs to make that one compile so i didn't check further) and some existing shaders and/or multimesh usage out there might rely on the current assumption: e.g. a shader that was getting white/ones might suddenly get zeroes and cause stuff to disappear/go black by upgrading Godot (or the opposite).

---

Regardless of how this will be solved (might need some discussion - or perhaps use the gridmap-specific bug but also make a bug for the multimesh one since even after fixing the multimesh bug the observed behavior wont change and we can remove the workaround in gridmap), i think this functionality will be very useful with a bunch of use cases.

However i think it'll also need some editor support to be able to set colors and data from the editor - e.g. setting color variations, data for small things like adding fake leaf/grass wind motion (and using the custom data for the strength), setting up rates for damage/loot/etc and other things. This is something that should be done properly after the proposal i commented on here godotengine/godot-proposals#11206 (comment) is implemented (with a "set data/color" tool that has a panel that lets the user specify data and color) but until that it could also be implemented using a custom game-specific editor plugin after #99639 is merged (so, e.g, the user can select the cells they want to modify and use their plugin to setup specific parameters for them).