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

Update CanvasLayer 'Follow Viewport' documentation to match its behavior #99754

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

kthang55
Copy link
Contributor

@kthang55 kthang55 commented Nov 27, 2024

Fixes #98463 by updating the documentation for the 'Follow Viewport' property to accurately describe its current behavior.

@AThousandShips
Copy link
Member

This should be fixed by adjusting the documentation IMO, this breaks compatibility in a major way

@kthang55
Copy link
Contributor Author

Fair enough, how should I best proceed? Reverting the changes and making another commit with changes in the documentation? Or should I make a new PR altogether?

@AThousandShips
Copy link
Member

You can update this PR

@kthang55 kthang55 requested a review from a team as a code owner November 27, 2024 13:06
@AThousandShips AThousandShips added enhancement documentation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release and removed discussion breaks compat topic:gui labels Nov 27, 2024
@AThousandShips AThousandShips changed the title Fix CanvasLayer 'Follow Viewport' behavior to match its intended purpose Update CanvasLayer 'Follow Viewport' documentation to match its behavior Nov 27, 2024
@Mickeon
Copy link
Contributor

Mickeon commented Nov 27, 2024

I don't understand why the documentation change has been done in Parallax2D. It looks like the original issue's confusion stem from CanvasLayer. No mention of Parallax2D.

@kthang55
Copy link
Contributor Author

My mistake, this should be fixed now with the latest push :D

@Mickeon Mickeon requested a review from a team November 27, 2024 16:00
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

This is not the first time users have gotten confused by this setting. I've seen it crop up sporadically in conversation that it should be the opposite.
So maybe this makes more sense, but it would be nice to factually check this.

@Meorge
Copy link
Contributor

Meorge commented Nov 27, 2024

For reference, this is what it currently says (in case it gets updated in the future):

remains anchored in a fixed position on the screen and does not move with the camera. If disabled, the [CanvasLayer] will move along with the camera's transform

This wording still feels ambiguous to me, like both descriptions for true/false are saying the same thing.

"Remains anchored in a fixed position on the screen and does not move with the camera" sound contradictory - if it remains in a fixed position on the screen (not the world), but the camera moves, then it should look like it is moving with the camera within the world space, because the camera is our view into the world.

"Moving along with the camera's transform" sounds to me like, if you scroll the camera to the right for example, the CanvasLayer will move to the right as well ("along with the camera's transform"), and thus appear to be in a fixed position on the screen.

@kthang55
Copy link
Contributor Author

What about something like this?
"If enabled, the [CanvasLayer] stays in a fixed position relative to the screen, ignoring camera movement.
If disabled, the [CanvasLayer] moves along with the camera, maintaining its position in world space."

@Meorge
Copy link
Contributor

Meorge commented Nov 27, 2024

I'm thinking that sounds good, but perhaps with "ignoring camera movement" and "moves along with the camera" removed? Simply stating "if enabled, it stays in a fixed position on the screen; if disabled, it maintains its position in world space" seems clearest to me.

@kthang55
Copy link
Contributor Author

That sounds good, this would be the full documentation for it then:
"If enabled, the [CanvasLayer] stays in a fixed position on the screen.
If disabled, the [CanvasLayer] maintains its position in world space.
Together with [member follow_viewport_scale], this can be used for a pseudo-3D effect."
Does that look good?

@Meorge
Copy link
Contributor

Meorge commented Nov 27, 2024

Sounds good to me personally! It may be good to check with others and ensure it sounds good to them too, though.

@kthang55
Copy link
Contributor Author

Pushed the new change and awaiting further feedback!

@Mickeon
Copy link
Contributor

Mickeon commented Nov 27, 2024

The updated description is still fine by me, assuming accuracy. I would suggest updating your PR's own description as it's outdated.

@kthang55
Copy link
Contributor Author

kthang55 commented Nov 27, 2024

Would that be the comment that is at the top? The one starting with "Fixes #98463..."?

Edit: It has been changed

@akien-mga akien-mga removed this from the 4.x milestone Nov 28, 2024
@akien-mga akien-mga added this to the 4.4 milestone Nov 28, 2024
@akien-mga
Copy link
Member

Rebased to squash the commits, should be ready to merge.

@akien-mga akien-mga merged commit 0b2b8f0 into godotengine:master Nov 29, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@kthang55
Copy link
Contributor Author

Thanks!

@kthang55 kthang55 deleted the fix_follow_viewport branch November 30, 2024 00:12
@rt9391
Copy link

rt9391 commented Feb 19, 2025

The description does not match the actual behavior, in fact it does the opposite.

I made a project here.
"If enabled, the CanvasLayer stays in a fixed position on the screen. If disabled, the CanvasLayer maintains its position in world space."
The GUI nodes (those labels) is in a canvaslayer that DISABLED follow_viewport_enabled, whitch would be "maintains its position in world space" according to the doc.
But apparently these GUI only "fixed position on the screen", not maintains in wolrd space.

CanvasLayerFviewTest.zip

MRP file:

QQ.20250219174931.mp4

rt9391 added a commit to rt9391/rt9391godot2 that referenced this pull request Feb 21, 2025
The doc of follow_viewport_enabled in godotengine#99754 tells the opposite thing.
This PR swaped enabled/disabled description to match actual behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canvas Layer "Follow Viewport" property does the opposite of what it is supposed to
6 participants