Fix `Parallax2D` repeats being not relative to its transform

[kleonc]

trafficstars

Fixes #95574.

To me the previous behavior seems bugged (hence marking it as such) but I'm not fully sure about it.

@markdibarry Not sure if that's the behavior you suggested in https://github.com/godotengine/godot/issues/95574#issuecomment-2292447062 though. So feeeback welcomed. :slightly_smiling_face: Also if that seems like what's desired then please test it further to ensure no regressions (I'm assuming you have plenty of test projects for Parallax2D).

Project with examples presented below: parallax_demo_MRP.zip

Original	repeat setup	Before (v4.3.stable.official [77dcf97d8])	After (this PR)

[markdibarry]

This is great! I was working on this right now, and I was thinking of similar changes as well. It seems you have the same issue I'm tackling too, the culling still is expecting the transform to follow the original, so it breaks:

That being said, I agree with the change to the do-while mainly because I just hated having that there at all. Second, if we're passing the source canvas item, might as well remove the repeat size and times params since we can access them directly on the source.

Thanks for digging in with me on some nagging issues!

[kleonc]

It seems you have the same issue I'm tackling too, the culling still is expecting the transform to follow the original, so it breaks:

Indeed I haven't looked into rects/clipping (yet!). :upside_down_face: Will do so tomorrow and update this.

Second, if we're passing the source canvas item, might as well remove the repeat size and times params since we can access them directly on the source.

Duh! :man_facepalming:

[markdibarry]

This also would work in the renderer and would prevent us from needing to add even more bloat to CanvasItem, but I'm not sure if there's a better way or how bad this is performance-wise compared to passing the source item everywhere

Transform2D base_transform;

if (p_offset.x || p_offset.y) {
	Transform2D new_transform = p_item->xform_curr;
	new_transform.set_origin(new_transform.get_origin() + p_offset);
	new_transform = p_item->final_transform * p_item->xform_curr.affine_inverse() * new_transform;
	base_transform = p_canvas_transform_inverse * new_transform; // TODO: Interpolate or explain why not needed.
} else {
	base_transform = p_canvas_transform_inverse * p_item->final_transform;
}

[kleonc]

This also would work in the renderer and would prevent us from needing to add even more bloat to CanvasItem, but I'm not sure if there's a better way or how bad this is performance-wise compared to passing the source item everywhere

Transform2D base_transform;

if (p_offset.x || p_offset.y) {
	Transform2D new_transform = p_item->xform_curr;
	new_transform.set_origin(new_transform.get_origin() + p_offset);
	new_transform = p_item->final_transform * p_item->xform_curr.affine_inverse() * new_transform;
	base_transform = p_canvas_transform_inverse * new_transform; // TODO: Interpolate or explain why not needed.
} else {
	base_transform = p_canvas_transform_inverse * p_item->final_transform;
}

This would kinda work for direct children of the repeat source (Parallax2D), but not in general case. The offset needs to be applied relative to the repeat source (Parallax2D node)(hence why I concluded simplest would be to store a reference to it). Try the animated scene example from the project I've attached, won't work correctly.

Moreover xform_curr is potentially different than the actual current transform because of physics interpolation. Aka using it for calculations would introduce additional discrepancies (thus "would kinda work").

final_transform which I've used for calculations already incorporate physics interpolation, so there shouldn't be issues with it (haven't actually tested it with physics interpolation).

Also for reference my current calculation:

Transform2D base_transform = p_item->final_transform;
if (p_item->repeat_source_item && (p_repeat_offset.x || p_repeat_offset.y)) {
	base_transform.columns[2] += p_item->repeat_source_item->final_transform.basis_xform(p_repeat_offset);
}
base_transform = p_canvas_transform_inverse * base_transform;

is equivalent to / a simplified version of:

Transform2D base_transform = p_item->final_transform;
if (p_item->repeat_source_item && (p_repeat_offset.x || p_repeat_offset.y)) {
	Transform2D source_xform = p_item->repeat_source->final_transform;
	base_transform = source_xform * Transform2D(0, p_repeat_offset) * source_xform.affine_inverse() * base_transform;
}
base_transform = p_canvas_transform_inverse * base_transform;

[kleonc]

Updated the rect calculation, further testing welcomed. :slightly_smiling_face: If I got it right it should result in the white rect (contrary to the blue one which is seemingly simpler to obtain):

[markdibarry]

Doing some general performance tests before/after.

I agree that it's better to go add more stuff and do it the "right way" rather than hacking it with the interpolation data. It's beneficial to have the repeat_source_item anyway, just because that opens us up to being able to support nested Parallax2D eventually. I just always take pause when I'm throwing more logic and stuff onto objects in a hot-path.

[markdibarry]

Also looks like the culling is correct. CanvasGroup is so good for visualizing it:

https://github.com/user-attachments/assets/444b6854-1711-4408-9507-6466b142abd4

[markdibarry]

Hm. Unfortunately, this seems to break ParallaxBackground and ParallaxLayers, which seem to not repeat anymore. We'll have to make sure there's no regressions there.

[markdibarry]

@kleonc As one solution, we could just have ParallaxLayer store data the same as Parallax2D. I don't see any side effects from any of my tests with the following changes:

parallax_layer.cpp line 65-78

void ParallaxLayer::_update_mirroring() {
	if (!is_inside_tree()) {
		return;
	}

	ParallaxBackground *pb = Object::cast_to<ParallaxBackground>(get_parent());
	if (pb) {
		RID c = pb->get_canvas();
		RID ci = get_canvas_item();
		Point2 mirror_scale = mirroring * orig_scale;
		RenderingServer::get_singleton()->canvas_set_item_repeat(ci, mirror_scale, 1);
		RenderingServer::get_singleton()->canvas_item_set_interpolated(ci, false);
	}
}

renderer_canvas_cull.cpp line 53-55

for (int i = 0; i < p_child_item_count; i++) {
	_cull_canvas_item(p_child_items[i].item, p_transform, p_clip_rect, Color(1, 1, 1, 1), 0, z_list, z_last_list, nullptr, nullptr, true, p_canvas_cull_mask, Point2(), 1, nullptr);
}

We'd also be free to remove canvas_set_item_mirroring() from the rendering server in that case as well.

[markdibarry]

Also, did some testing with 40 Parallax2Ds, with 5 repeats each (You shouldn't need it higher than 3, though I know people will use 20 if they can) I was getting between 7500-7000 FPS on my machine and no noticeable change between branches.

Since taking advantage of the changes to the parallax logic, there's similar performance for ParallaxBackground and ParallaxLayer as well. Just out of curiousity, I did a similar test for 40 ParallaxLayers on 4.2 (obviously without the 5 repeats) and it was somewhere around 1800 FPS, so... wow!

[kleonc]

Hm. Unfortunately, this seems to break ParallaxBackground and ParallaxLayers, which seem to not repeat anymore. We'll have to make sure there's no regressions there.

I guess I forgot they exist. :laughing: But to be clear you're saying they should repeat the same way as Parallax2D with this PR, not in the pre-this-PR manner?

@kleonc As one solution, we could just have ParallaxLayer store data the same as Parallax2D. I don't see any side effects from any of my tests with the following changes:

parallax_layer.cpp line 65-78

void ParallaxLayer::_update_mirroring() {
	if (!is_inside_tree()) {
		return;
	}

	ParallaxBackground *pb = Object::cast_to<ParallaxBackground>(get_parent());
	if (pb) {
		RID c = pb->get_canvas();
		RID ci = get_canvas_item();
		Point2 mirror_scale = mirroring * orig_scale;
		RenderingServer::get_singleton()->canvas_set_item_repeat(ci, mirror_scale, 1);
		RenderingServer::get_singleton()->canvas_item_set_interpolated(ci, false);
	}
}

renderer_canvas_cull.cpp line 53-55

for (int i = 0; i < p_child_item_count; i++) {
	_cull_canvas_item(p_child_items[i].item, p_transform, p_clip_rect, Color(1, 1, 1, 1), 0, z_list, z_last_list, nullptr, nullptr, true, p_canvas_cull_mask, Point2(), 1, nullptr);
}

Currently (pre-this-PR) mirror set in RenderingServer.canvas_set_item_mirroring is used only as the initial value for _cull_canvas_item's p_repeat_size argument: https://github.com/godotengine/godot/blob/1bd740d18d714f815486b04bf4c6154ef6c355d9/servers/rendering/renderer_canvas_cull.cpp#L54

Meaning if a canvas item is set as repeat source (e.g. via directly calling RenderingServer.canvas_set_item_repeat) then it would already ignore such mirroring. So there's a potential clash between repeat/mirroring already, thus I agree it makes sense to make both use repeat within the RenderingServer (seems they are already not meant to be used together).

But instead of changing ParallaxLayer to call RenderingServer.canvas_set_item_repeat I think it makes more sense to change RenderingServer.canvas_set_item_mirroring to do the same as RenderingServer.canvas_set_item_repeat (or just call it). And remove the mirror from the RenderingServer internals as it would be no longer used.

We'd also be free to remove canvas_set_item_mirroring() from the rendering server in that case as well.

While I agree that RenderingServer.canvas_set_item_mirroring seems absolutely useless, I don't think we can just remove it, would be compatibility breaking. But I think deprecating it should be fine, and the docs could be updated to recommend using RenderingServer.canvas_set_item_repeat instead (as it would be a wrapper for it anyway).

Also, did some testing with 40 Parallax2Ds, with 5 repeats each (You shouldn't need it higher than 3, though I know people will use 20 if they can) I was getting between 7500-7000 FPS on my machine and no noticeable change between branches.

Kinda expected no noticeable change as not doing anything heavy in here, but great to see a confirmation!

Since taking advantage of the changes to the parallax logic, there's similar performance for ParallaxBackground and ParallaxLayer as well. Just out of curiousity, I did a similar test for 40 ParallaxLayers on 4.2 (obviously without the 5 repeats) and it was somewhere around 1800 FPS, so... wow!

Comparing with the numbers from https://github.com/godotengine/godot/pull/87391#issue-2091849239 seems like it got better in the meantime, great to see!

@markdibarry Massive thanks for testing/feedback! :slightly_smiling_face:

[markdibarry]

I guess I forgot they exist. 😆 But to be clear you're saying they should repeat the same way as Parallax2D with this PR, not in the pre-this-PR manner?

This is a regression in ParallaxLayer and an oversight in Parallax2D. In 4.2 ParallaxLayer behaved like Parallax2D does after this PR, so it should definitely get the behavior back.

Currently (pre-this-PR) mirror set in RenderingServer.canvas_set_item_mirroring is used only as the initial value for _cull_canvas_item's p_repeat_size argument:

Yeah, that's why I replaced it with a default value in my suggestion. I originally passed in the mirroring so the old nodes could use the new system, but adding a source item parameter complicates things. So whether copying the new repeat method logic to the old mirror method or calling the new one, it should have the same result. I'll trust your decision.

But instead of changing ParallaxLayer to call RenderingServer.canvas_set_item_repeat I think it makes more sense to change RenderingServer.canvas_set_item_mirroring to do the same as RenderingServer.canvas_set_item_repeat (or just call it). And remove the mirror from the RenderingServer internals as it would be no longer used.

That's a good point. I think the plan is to deprecate on the next version, and promote Parallax2D to non-experimental, so that does make more sense. Lets go with that!

Comparing with the numbers from https://github.com/godotengine/godot/pull/87391#issue-2091849239 seems like it got better in the meantime, great to see!

Well, it's semi-arbitrary, since an empty scene is 8000 FPS for my machine now, and that could be because I've got a new compy, but also could be multiple other variables. Either way, seeing that there's more than 4x improvement between versions for heavier scenes even for the old system is very promising.

[markdibarry]

If you fix the repeat_times issue, I can't find any other problems. LGTM!

[kleonc]

This is a regression in ParallaxLayer and an oversight in Parallax2D. In 4.2 ParallaxLayer behaved like Parallax2D does after this PR, so it should definitely get the behavior back.

Oh, indeed that's a regression! Another good catch. :+1: So definitely this PR should go into 4.3.x (was going to mark it for cherry-picking anyway).

Yeah, that's why I replaced it with a default value in my suggestion. I originally passed in the mirroring so the old nodes could use the new system, but adding a source item parameter complicates things. So whether copying the new repeat method logic to the old mirror method or calling the new one, it should have the same result. I'll trust your decision.

Same here, I also tried passing the old mirror, and concluded that making it use repeating instead was more sensible than patching things along the way. Above I've added some justification for why I think it's fine to do so (and almost for sure shouldn't break anything) only for future reference.

That's a good point. I think the plan is to deprecate on the next version, and promote Parallax2D to non-experimental, so that does make more sense. Lets go with that!

For sure Parallax2D would need to be marked as non-experimental before deprecating ParallaxLayer/ParallaxBackground (or at the same time). When exactly it's planned to be done I don't know, but makes sense to me to already deprecate RenderingServer.canvas_set_item_mirroring. Will probably open a separate PR for that later (not meant to be cherry-picked into 4.3.x).

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 4.3.1.