godot
godot copied to clipboard
Avoid passing zoom scale for ParallaxLayer mirror
ParallaxBackground
doesn't support zoom, so instead it scales all the ParallaxLayer
s 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:
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.
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
It would be good to figure out what commit caused the regression, since it's apparently not the addition of Parallax2D.
Oh that's very interesting, sounded like it was that, will do some digging
@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.
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
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
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
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.
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.