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

Improve docs on how ParallaxLayer mirroring works. #80896

Merged

Conversation

lvella
Copy link
Contributor

@lvella lvella commented Aug 22, 2023

I wrote everything I wish was written when I tried to make an infinite scrolling background, addressing every point of every mistake I made.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

As expressed on your original PR this feels too big to fit in this section, and belongs in a tutorial in my opinion

doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
@lvella
Copy link
Contributor Author

lvella commented Aug 26, 2023

Thanks for reviewing.

As expressed on your original PR this feels too big to fit in this section, and belongs in a tutorial in my opinion

I believe the reference docs should be the complete and authoritative source of information. I.e. an official tutorial should bring no more information than what you can piece together from the references.

Of all I wrote, I think only this paragraph doesn't bring novel information important to the motion_mirroring property. It is more like an usage tip:

Notice that if you want the repeatition to pixel-perfect match a [Texture2D] displayed by a child node, you should account for any scale applied to the texture when defining this interval. E.g., if you use a child [Sprite2D] scaled to [code]0.5[/code] to display a 600x600 texture, and want this sprite to be repeated continuously horizontally, you should set the mirroring to [code]Vector2(300, 0)[/code].

But even then, I don't think such an usage tip is out of place in a reference documentation: the reference should be more complete and thorough on the details than a tutorial, not less.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Some style and grammar pointers

Unsure about the verbosity but some good pointers, I think some of the details might be best served in a detailed tutorial explaining things with examples, but works here as well

doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
@lvella
Copy link
Contributor Author

lvella commented Sep 13, 2023

Sorry for the long delay. I applied all the fixes, thanks.

@lvella lvella force-pushed the parallax-layer-mirroring-explained branch from 7d50c89 to 996714c Compare September 30, 2023 13:25
@lvella lvella marked this pull request as draft September 30, 2023 13:44
@lvella lvella force-pushed the parallax-layer-mirroring-explained branch from 996714c to 7d50c89 Compare September 30, 2023 13:51
@lvella lvella marked this pull request as ready for review September 30, 2023 14:09
Copy link
Contributor Author

@lvella lvella left a comment

Choose a reason for hiding this comment

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

typo

doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
@lvella
Copy link
Contributor Author

lvella commented Oct 2, 2023

How can CI fail in building C++ code if I didn't touch it?

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.

Neat details to a class that may have desperately needed some, although perhaps it's a little too much. I have reviewed this just when a new Parallax2D node is being in the works so this may be a bit obsolete, but it is nice nonetheless.

doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
doc/classes/ParallaxLayer.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga changed the title Better explaining how mirroring works. Improve docs on how ParallaxLayer mirroring works. Feb 14, 2024
I wrote everything I wish was written when I tried to make an
infinite scrolling background, addressing every point of every
mistake I made.
@akien-mga akien-mga force-pushed the parallax-layer-mirroring-explained branch from e8c9c0b to 40813b6 Compare February 14, 2024 12:08
@akien-mga
Copy link
Member

Committed @Mickeon's suggestions and squashed the commits.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 14, 2024
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Feb 14, 2024
@akien-mga akien-mga merged commit 5b56f37 into godotengine:master Feb 14, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Mar 11, 2024
@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.
Cherry-picked for 4.1.4.

@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Mar 11, 2024
@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
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.

4 participants