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.

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

Fix CanvasModulate logic for modulating the canvas #79747

Merged

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Jul 21, 2023

CanvasModulate can modulate the canvas only when it is in some canvas and it is visible in the tree.

Previously CanvasModulate would update / try to update the canvas modulate color (within the RenderingServer) always on:

  • entering/exiting the canvas,
  • visibility changed notificiation.

This can fail in many different ways.

Some of them in v4.1.1.stable.official [bd6af8e] (click to expand).

  • Failing to obtain the canvas when out of tree:
extends Node2D

func _ready() -> void:
	var cm := CanvasModulate.new()
	cm.visible = false

Godot_v4 1 1-stable_win64_8GLsA27SNP

  • "Fighting" for changing the canvas modulate color (last one changed takes over):
    yUoB12a0u3

  • Changing canvas modulate color regardless of other CanvasModulates:
    TEXd9UxoPG
    G0VdFiGJSa
    (on deleting:)
    Nlyt5ND5uG

Etc.


This PR makes exactly one CanvasModulate be active (affecting the canvas modulation) in each (non-empty) canvas modulate group.

  • When for a CanvasModulate the (is_in_canvas, is_visible_in_tree) tuple changes to / from (true, true), it's being added to / removed from the canvas modulate group.
  • When a CanvasModulate is added to the canvas modulate group, it becomes active only if the group was empty. Otherwise, there already was an active CanvasModulate in such group and it remains the active one.
  • When an active CanvasModulate is removed from the canvas modulate group, it activates another CanvasModulate from the group (or restores canvas to no modulation if there are no more CanvasModulates left in the group).

Note that for more than one CanvasModulate in the given canvas modulate group it's said it's undefined which one is active. It could be implemented that the active one is always e.g. the topmost according to the tree order, or the one added to the canvas modulate group most/least recently or something like that. However, more than one CanvasModulate in such group is considered an invalid state, hence I find it not worth the additional burden (e.g. for tree ordering CanvasModulates would need to react to parent's child_order_changed). I think what's important is that after ending with a single CanvasModulate within such group, it should work as expected. This should be the case with this PR.

Fixes #73894.
Fixes #79679.


I think it's good for cherry-picking even if it possibly changes the previous buggy behavior for more than one CanvasModulate in the canvas. That's invalid state which shouldn't be relied upon and the previous message within the in-editor warning:

Only one visible CanvasModulate is allowed per scene (or set of instantiated scenes). The first created one will work, while the rest will be ignored.

was misleading / not true anyway.

@kleonc kleonc added this to the 4.2 milestone Jul 21, 2023
@kleonc kleonc requested a review from a team as a code owner July 21, 2023 12:51
@kleonc kleonc added cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 21, 2023
@akien-mga akien-mga merged commit 132b97c into godotengine:master Aug 16, 2023
@akien-mga
Copy link
Member

Thanks!

@kleonc kleonc deleted the canvas_modulate_fix_updating_logic branch August 16, 2023 08:46
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2. That's a bigger change, but I'm willing to give it a go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CanvasModulate not remembering what color assigned to Hidden CanvasModulate raise errors
4 participants