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

Add emit_changed() to Environment setters #99873

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ramokz
Copy link

@ramokz ramokz commented Nov 30, 2024

Similar to #99729, added missing emit_changed() to the Environment resource setters.

Attached a MRP for testing:
cam_env.zip

Steps to reproduce / test:

  1. Run the scene
  2. Select the Camera3D node from the Scene Tree
  3. Expand the Environment resource attached to the Camera3D
  4. Modify Environment properties
  5. Observe the Output console emitting a print statement whenever a value is changed.

@ramokz ramokz requested review from a team as code owners November 30, 2024 13:27
@AThousandShips AThousandShips changed the title Added emit_changed() to Environment setters Add emit_changed() to Environment setters Nov 30, 2024
@Calinou Calinou added bug topic:rendering topic:3d cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 30, 2024
@Calinou Calinou added this to the 4.4 milestone Nov 30, 2024
@clayjohn
Copy link
Member

Just a heads up, we intentionally don't call emit_changed() on all setters. This topic has come up multiple times over the years and every time the maintainers have agreed to expose it for functions where users have expressed that they need it and shown that they do indeed need it.

We explicitly do not want to just add it everywhere since it would be adding performance overhead for little benefit.

To that end, can you provide a bit of background about your project and where/why you need the emit_changed signal?

@ramokz
Copy link
Author

ramokz commented Nov 30, 2024

It's to add functionality and avoid an expensive workaround for a camera addon that I maintain to allow for pseudo camera nodes to affect a scene's Camera3D Environment resource. More specifically, it relates to this issue.

In the context of the project, the intent is when you modify one of the pseudo camera's Environment resources, it will then update the scene's Camera3D Environment resource — effectively staying in sync.

At the minute, and only when the scene is not playing, I'm resorting to duplicating the pseudo camera's Environment resource directly to the Camera3D in the _process function, as there's currently no way to detect whenever a value within it has changed. The changes could happen at any time, so the current approach here is purely to keep the resource values synchronized between the two nodes that hold that resource instance.

The workaround is obviously far from a performant approach, since the values don't change regularly enough to justify assigning the values every frame; hence why it's not being triggered during runtime and only in the editor. But that leaves the issue of it being a significant overhead in the editor and a non-existent functionality during runtime; both not being ideal. Hence, the emit_changed() for this particular resource.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:rendering topic:3d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants