godot icon indicating copy to clipboard operation
godot copied to clipboard

Avoid passing zoom scale for ParallaxLayer mirror

Open markdibarry opened this issue 11 months ago • 9 comments

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:

https://github.com/godotengine/godot/assets/21325943/499ba4fe-4c20-4f3a-92e0-90bdebab965b

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.

markdibarry avatar Mar 16 '24 15:03 markdibarry

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

AThousandShips avatar Mar 16 '24 15:03 AThousandShips

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

akien-mga avatar Mar 16 '24 15:03 akien-mga

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

AThousandShips avatar Mar 16 '24 15:03 AThousandShips

@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.

markdibarry avatar Mar 16 '24 15:03 markdibarry

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

AThousandShips avatar Mar 16 '24 15:03 AThousandShips

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:

https://github.com/godotengine/godot/assets/21325943/ff777b29-6b79-4925-a7e3-4c462276d460

Zoom scrolls textures:

https://github.com/godotengine/godot/assets/21325943/3c1db403-dfe1-4774-a98e-d4ecdf1cdc0c

markdibarry avatar Mar 16 '24 16:03 markdibarry

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

Mickeon avatar Mar 16 '24 17:03 Mickeon

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.

markdibarry avatar Mar 18 '24 14:03 markdibarry

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.

Pshy0 avatar Mar 18 '24 21:03 Pshy0