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

Avoid passing zoom scale for ParallaxLayer mirror #89572

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

markdibarry
Copy link
Contributor

ParallaxBackground doesn't support zoom, so instead it scales all the ParallaxLayers up(!). The old rendering method required that calculation done up front when deciding repeat size, but the new one doesn't. This just uses the ParallaxLayer's original scale to mirror instead.

Before:
68747470733a2f2f692e6779617a6f2e636f6d2f30326364616462626464653565383332643933613765366232356337346561392e676966

After:

2024-03-16.10-43-57.mp4

Fixes #89563

I'd like to have some further testing to avoid more regressions. My original PR didn't have any changes to ParallaxBackground or ParallaxLayer so I want to be cautious with this fix. I'll comment when I have tried a few more scenarios.

@AThousandShips AThousandShips requested a review from a team March 16, 2024 15:10
@AThousandShips
Copy link
Member

AThousandShips commented Mar 16, 2024

The old rendering method required that calculation done up front when deciding repeat size, but the new one doesn't

This is somewhat concerning and needs some deeper investigation of potential other side effects from the changes, and sounds like a compatibility breakage (and that method is exposed)

I'm not sure this is the right way to solve this, unsure that breakage is considered valid

@akien-mga
Copy link
Member

It would be good to figure out what commit caused the regression, since it's apparently not the addition of Parallax2D.

@AThousandShips
Copy link
Member

Oh that's very interesting, sounded like it was that, will do some digging

@markdibarry
Copy link
Contributor Author

markdibarry commented Mar 16, 2024

@akien-mga Sorry, I caused a misunderstanding: It is my original PR that caused it. I was just clarifying that I didn't change any ParallaxBackground or ParallaxLayer code in my PR. The renderer method handles both the old nodes and the new one. It used to require the ParallaxLayer to account for the scaling before it sent its size (now it doesn't), but now that I'm actually modifying something in ParallaxLayer I want to be sure it's the right call.

@AThousandShips
Copy link
Member

Now this feature isn't used by the engine anywhere else, but it is exposed, so needs some evaluation of what to do to possibly preserve previous behavior

@markdibarry
Copy link
Contributor Author

I've been trying a couple more scenarios between 4.2 and this PR and so far it looks like I haven't found any other regressions in the old nodes. This does, however, uncover an interesting choice we have.

The behavior for ParallaxBackground is that when you zoom in/out, it scrolls the textures as well. I'm not sure if this is desired or not. If not, no changes need be made. If so, for Parallax2D, you would have to pass the camera's zoom value separately for every update. The reason is that using the camera's transform would take rotation into account, which we definitely don't want to do with Parallax2D. This was not an issue with ParallaxBackground only because it doesn't support rotation at all. I could also provide both as a selectable option, but I'd like to know what other's think.

Zoom doesn't scroll textures:

2024-03-16.12-20-33.mp4

Zoom scrolls textures:

2024-03-16.12-17-38.mp4

@Mickeon
Copy link
Contributor

Mickeon commented Mar 16, 2024

The behavior for ParallaxBackground is that when you zoom in/out, it scrolls the textures as well. I'm not sure if this is desired or not.

Absolutely not in my humble opinion. Feels like yet another reason to favour Parallax2D

@markdibarry
Copy link
Contributor Author

I did some further testing comparing between versions and it seems this does fix the regression. Unfortunately, the original behavior is not the most ideal since every time you update the scale, you need to reset the mirroring property or it's off. It also seems that ParallaxLayer never fully supported scaling besides the zoom feature and scrolling will break if you alter it manually. It's not hard to fix, but that should be in a separate PR. I encountered this issue as well in Parallax2D, so I'll make a quick PR to fix that too.

@Pshy0
Copy link

Pshy0 commented Mar 18, 2024

When zoom scrolls textures, it does not look like a "zoom", but rather like moving the camera forward.
Perhaps there could be another property for that if this is needed.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Seems fine to me, but I am not very familiar with the ParallaxLayer code

@akien-mga akien-mga requested a review from kleonc June 15, 2024 08:58
@akien-mga akien-mga merged commit 9dca899 into godotengine:master Jun 17, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@markdibarry markdibarry deleted the parallax_scale_regression branch June 18, 2024 14:22
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.

Regression in ParallaxBackground
6 participants