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

Make SubViewportContainer's adjustments optional #71227

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

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jan 11, 2023

This PR improves documentation about properties modified by SubViewportContainer and makes this behavior optional by adding adjust_viewport_properties property.
Closes #56502
Closes #23729

@KoBeWi KoBeWi added this to the 4.x milestone Jan 11, 2023
@KoBeWi KoBeWi requested review from a team as code owners January 11, 2023 15:41
@kubecz3k
Copy link
Contributor

kubecz3k commented Jan 11, 2023

I would argue it probably would be better to simply the behavior by ensuring Godot is not changing the value of update_mode every time the visibility is changing.
It should either:

  • simply disable the processing using internal flag which is not exposed to gdscript
  • or don't do anything since this can be achieved very easily manually in gdscript (and that's how I would expect it to work before this pr without looking at the implementation) (edit: after reconsideration I think it's not so obvious as it seemed to me at first)

@KoBeWi
Copy link
Member Author

KoBeWi commented Jan 11, 2023

simply disable the processing using internal flag which is not exposed to gdscript

Update mode is handled on RenderingServer side. It can't be an internal flag.

EDIT:
Ah, I think I misunderstood. It could be an internal flag. The problem is that Viewport and SubViewport would need additional property that would only be modified from SubViewportContainer (and it would basically duplicate existing properties). It would add complexity for little gain. The changes in my PR are simpler and basically solve the same problem.

@kubecz3k
Copy link
Contributor

kubecz3k commented Jan 11, 2023

Well from user perspective internal flag makes it behave exactly like they would expect (disabling processing when SubViewportContainer is hidden), while respecting update_mode (in current behavior exposing update_mode has no sense since no matter what programmer will set there, engine will always overwrite it). This way we would not need to introduce new a bit cryptic adjust_viewport_properties flag and as a result the api would be simpler/cleaner

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 30, 2023

Rebased. I also tweaked _validate_property() to disable the properties handled by the container.

EDIT:
I also made the adjustments take effect right after SubViewport is added to the container.

@KoBeWi KoBeWi modified the milestones: 4.x, 4.3 Oct 30, 2023
@KoBeWi KoBeWi force-pushed the magic_behavior branch from f43d910 to 33b6eba Compare May 5, 2024 23:30
@akien-mga akien-mga modified the milestones: 4.3, 4.x Jun 26, 2024
Deltt added a commit to Deltt/godot that referenced this pull request Dec 5, 2024
Removes potentially undesired resets of render_target_update_mode,

see godotengine#23729 and godotengine#71227
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants