Add a method to get global modulate #70294
Conversation
| @@ -388,6 +388,16 @@ Color CanvasItem::get_modulate() const { | |||
| return modulate; | |||
| } | |||
|
|
|||
| Color CanvasItem::get_modulate_in_tree() const { | |||
There was a problem hiding this comment.
hmm. That sounds a bit inefficient no? This modulate value is likely already present in the engine. I seems there's a final_modulate value accessible in the rendering server. In renderer_canvas_cull.cpp. You may try something like that:
// In the CanvasItem code you can get the CanvasItem RID
RID rid = get_canvas_item()
// And fetch the final_modulate value in the rendering server code I guess ?
Item *item = canvas_item_owner.get_or_null(rid);
return item->final_modulate
There was a problem hiding this comment.
RenderingServer doesn't seem to have any similar getter method and I didn't want to touch this code.
I think given the current usage, the method doesn't need to be super efficient for now.
There was a problem hiding this comment.
Yeah, but you know how it is... It's not exposed right now but as soon as someone will find it they'll ask themselves "why is this not exposed ?" and make a PR to expose it. And then we would have to modify it to be efficient enough.
If you prefer keeping it less efficient but not touch the rendering server, then keep it local to TileMap, where it's used. That will avoid us issues IMO.
There was a problem hiding this comment.
Well, such thing did not happen to the PR I linked in the OP (but that one is actually efficient). Even if we expose this one, it could be documented as not super efficient 🤔
Not sure about moving it to TileMap, because the method might be needed somewhere else eventually, but I could do that I guess.
There was a problem hiding this comment.
Well, such thing did not happen to the PR I linked in the OP (but that one is actually efficient). Even if we expose this one, it could be documented as not super efficient thinking
Yeah those ones are fine being exposed as we know they are efficient enough already I guess. At least I would put a comment there about not exposing this function without making it efficient first.
There was a problem hiding this comment.
To make it efficient, it could be cached and recomputed in NOTIFICATION_MOVED_IN_PARENT.
But we might want to want for @clayjohn's input to see if the value computed in the renderer could be exposed instead.
There was a problem hiding this comment.
Also the way the final modulate is being calculated in here is not correct, e.g. top level CanvasItems are not taken into account (chain should break on them). And there are also CanvasModulates, CanvasGroups, and maybe something else which would need a special treatment, not sure. I mean recalculating it in here is error prone, it does make more sense to obtain the actual modulate directly from the RenderingServer.
There was a problem hiding this comment.
top level CanvasItems are not taken into account
They are, check how get_parent_item() is implemented.
CanvasModulates
I mentioned it in the OP. It would require RenderingServer changes.
There was a problem hiding this comment.
Ok so I did the changes and final_modulate is just modulate * self_modulate. I don't know how to get the "final final" modulate, might be not available.
Here's patch with my attempt:
0001-Use-final-modulate.patch.txt
There was a problem hiding this comment.
@groud even if something is present in the rendering server, it absolutely should not be obtained on demand. As the server may be running threaded, it would block all processing. I think this function is ok.
| @@ -388,6 +388,16 @@ Color CanvasItem::get_modulate() const { | |||
| return modulate; | |||
| } | |||
|
|
|||
| Color CanvasItem::get_modulate_in_tree() const { | |||
There was a problem hiding this comment.
@groud even if something is present in the rendering server, it absolutely should not be obtained on demand. As the server may be running threaded, it would block all processing. I think this function is ok.
|
Thanks! |
Similar to #66546, this PR adds a (unexposed) method that returns modulate in node's tree branch. I wanted it to also use color from CanvasModulate, but there is no
canvas_get_modulate()method.I added a usage in TileMap editor to make the drawing previews use correct color.