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

Parallax2D (and ParallaxLayer) applies repeat to children nodes before their rotation #95574

Closed
squirrel-bat opened this issue Aug 15, 2024 · 5 comments · Fixed by #95666
Closed

Comments

@squirrel-bat
Copy link

Tested versions

v4.3.stable.official [77dcf97]

System information

Godot v4.3.stable - Pop!_OS 22.04 LTS - Wayland - GLES3 (Compatibility) - AMD Radeon Graphics (radeonsi, raphael_mendocino, LLVM 15.0.7, DRM 3.57, 6.9.3-76060903-generic) - AMD Ryzen 7 7800X3D 8-Core Processor (16 Threads)

Issue description

With the introduction of Parallax2D nodes, repeated children nodes will be placed in respect to their own rotation.
In the screenshot below, the sprite seen on the right is the repeat of the Sprite2D on the left side, with it's rotation set to 12° and Parallax2D's rotation left at 0°.

The same issue occurs with a setup of ParallaxBackground > ParallaxLayer > Sprite2D.

Screenshot from 2024-08-15 18-47-03

I am not sure if this is the intended behaviour? Please let me know if there is missing information or if I am misunderstanding how Parallax2D works! :)

Steps to reproduce

  • Create a Parallax2D node and set any Repeat Size with a minimum Repeat Times of 1
  • Add a child node (Sprite2D) and set it's rotation to anything other than 0°

Minimal reproduction project (MRP)

parallax_demo_MRP.zip

@squirrel-bat squirrel-bat changed the title Parallax2D applies repeat to children nodes before their rotation Parallax2D (and ParallaxLayer) applies repeat to children nodes before their rotation Aug 15, 2024
@markdibarry
Copy link
Contributor

markdibarry commented Aug 15, 2024

You know, I have no idea. I assumed it should be the same between the two versions, but I'm not sure what is desired. If it's more desired and doesn't have any bad side effects, I'd be happy to look into changing how it works.

Edit:
After our convo, I think you're right. We should explore this, but the behavior should be that the repeat should respect the item's rotation, just not when a child of a parallax node. I'll see if I can come up with something soon! Thanks for the great catch!

@kleonc
Copy link
Member

kleonc commented Aug 18, 2024

As pointed out in #95666 (comment) ParallaxLayer/ParallaxBackground behave the same way in v4.3.stable.official [77dcf97] which is a regression compared to v4.2.stable.official [46dc277]. Parallax2D was just added so not a regression but both me and @markdibarry consider this a bug. Hence marking as such.

v4.2.stable.official [46dc277] v4.3.stable.official [77dcf97]
V5Xq3RMX6w s4gOLbO21z

Edit: Just noticed this issue was mentioning ParallaxBackground/ParallaxLayer from the start, but I somehow got focused only on the Parallax2D part, my bad!

@akien-mga akien-mga added this to the 4.4 milestone Aug 19, 2024
@Macksaur
Copy link
Contributor

Macksaur commented Sep 13, 2024

I'm on 83d54ab and this seems to be still broken, I can't get the MRP from #95666 to produce the new behaviour. Actually, wait, it does work but only on Compatibility and does not work on Forward+.

@kleonc
Copy link
Member

kleonc commented Sep 13, 2024

I'm on 83d54ab and this seems to be still broken, I can't get the MRP from #95666 to produce the new behaviour. Actually, wait, it does work but only on Compatibility and does not work on Forward+.

Seems like #92797 was not properly rebased to account for the changes in servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp done in 1bd8372 / #95666. cc @stuartcarnie

Basically this part from the current master is pre-#95666 because of this:

Transform2D base_transform = p_canvas_transform_inverse * ci->final_transform;
if (!ci->repeat_size.x && !ci->repeat_size.y) {
_record_item_commands(ci, p_to_render_target, base_transform, current_clip, p_lights, instance_index, batch_broken, r_sdf_used);
} else {
Point2 start_pos = ci->repeat_size * -(ci->repeat_times / 2);
Point2 end_pos = ci->repeat_size * ci->repeat_times + ci->repeat_size + start_pos;
Point2 pos = start_pos;
do {
do {
Transform2D transform = base_transform * Transform2D(0, pos / ci->xform_curr.get_scale());
_record_item_commands(ci, p_to_render_target, transform, current_clip, p_lights, instance_index, batch_broken, r_sdf_used);
pos.y += ci->repeat_size.y;
} while (pos.y < end_pos.y);
pos.x += ci->repeat_size.x;
pos.y = start_pos.y;
} while (pos.x < end_pos.x);
}

So that's a new regression and, if anything, should be rather reported as a new issue.

@stuartcarnie
Copy link
Contributor

@kleonc thanks for directing me to the issue! It is fixed in #96977

stuartcarnie added a commit to stuartcarnie/godot that referenced this issue Sep 13, 2024
akien-mga added a commit that referenced this issue Sep 14, 2024
2D: Fix use-after-free in batch rendering; regression of #95574
hayahane pushed a commit to MonologistGames/godot that referenced this issue Sep 14, 2024
stuartcarnie added a commit to stuartcarnie/godot that referenced this issue Sep 15, 2024
laurentmackay pushed a commit to metapfhor/godot that referenced this issue Oct 30, 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 a pull request may close this issue.

7 participants