Skip to content
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.

Sign up for GitHub

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

Add a method to get global modulate #70294

Merged
merged 1 commit into from
Jan 21, 2023
Merged

Add a method to get global modulate #70294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion editor/plugins/tiles/tile_map_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,7 +922,7 @@ void TileMapEditorTilesPlugin::forward_canvas_draw_over_viewport(Control *p_over

// Get the tile modulation.
Color modulate = tile_data->get_modulate();
Color self_modulate = tile_map->get_self_modulate();
Color self_modulate = tile_map->get_modulate_in_tree() * tile_map->get_self_modulate();
modulate *= self_modulate;

// Draw the tile.
Expand Down
10 changes: 10 additions & 0 deletions scene/main/canvas_item.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,16 @@ Color CanvasItem::get_modulate() const {
return modulate;
}

Color CanvasItem::get_modulate_in_tree() const {
Copy link
Member

@groud groud Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@kleonc kleonc Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Color final_modulate = modulate;
CanvasItem *parent_item = get_parent_item();
while (parent_item) {
final_modulate *= parent_item->get_modulate();
parent_item = parent_item->get_parent_item();
}
return final_modulate;
}

void CanvasItem::set_as_top_level(bool p_top_level) {
if (top_level == p_top_level) {
return;
Expand Down
1 change: 1 addition & 0 deletions scene/main/canvas_item.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ class CanvasItem : public Node {

void set_modulate(const Color &p_modulate);
Color get_modulate() const;
Color get_modulate_in_tree() const;

void set_self_modulate(const Color &p_self_modulate);
Color get_self_modulate() const;
Expand Down