Multirect - Fix refining regions in derived Textures

[lawnjelly]

Fixes allowing all derived texture types to modify region prior to rendering.

Fixes similar bug to #76686 for LargeTexture and MeshTexture.

Notes

• Expands [3.x] Fix rendering tiles using nested AtlasTextures #76703 to deal with LargeTexture and MeshTexture, by returning an enum instead of a simple true / false, allowing fallback to the old draw_rect_region() function for complex texture types.
• Sends the input and output rect in the same arguments by reference. This could be marginally better at runtime than the separate input / output args (although the [3.x] Fix rendering tiles using nested AtlasTextures #76703 version separate args might be optimized out, I haven't examined assembly - it is quite possible they may not be optimized out as the call is virtual). On the flip side this does modify both rect and r in the calling code, although they don't appear to be used after the call to draw.
• Changes the function name, I wasn't quite sure the old name get_combined_rect_region made sense.

[kleonc]

Co-authored-by: kleonc kleonc@users.noreply.github.com

Note this doesn't link to my GH account, use 9283098+kleonc@users.noreply.github.com instead.

---

• Sends the input and output rect in the same arguments by reference. This could be marginally better at runtime than the separate input / output args (although the [3.x] Fix rendering tiles using nested AtlasTextures #76703 version separate args might be optimized out, I haven't examined assembly - it is quite possible they may not be optimized out as the call is virtual).

I like mutable/inout args (performance-wise they make more sense). They seem less readable on the caller side though. Note in #76703 I made args separated based on the Texture::get_region_rect which also passes in/out rects separately. If we're fine with mutable args then I wonder if it would make sense to change Texture::get_region_rect as well to use 2 args passed by non-const reference? 🤔

Other thing is I'm not sure about the naming convention for such params. Like what r_ prefix exactly stands for? I've always been thinking about it as "result"/"return". In such case it's kinda confusing / not obvious when such params act also as input. But maybe r_ is meant to mean "reference"? 🤔 (Doesn't make sense to me though, whether it's passed by (const/non-const) reference is already denoted by the type.)

On the flip side this does modify both rect and r in the calling code, although they don't appear to be used after the call to draw.

If the original values need to be preserved then copies could be passed in in the first place. So that's not an issue. Also it's even better to copy explicitely only when needed instead of always, possibly for no reason (and hoping compiler will optimize it out).

---

• Changes the function name, I wasn't quite sure the old name get_combined_rect_region made sense.

Haven't gave it much thought, I guess it could be get_final_rect_region, get_actual_rect_region, etc. Refine also doesn't seem like an obviously great name (it would be such if after seeing "refine" I'd know what exactly this method does - this is not the case for me). But (given I can't come up with a "great name") I don't think it matters much, in the end it's an internal method. I'm fine with "refine".

---

This version also currently makes no draw the default for all Texture derived classes if width or height is zero. This is currently the behaviour for ImageTexture and a few others, but not for the gradiant textures. I'm just trying to evaluate whether this would be a better all round default / whether it will change anything. If not it's fairly easy to flip the logic here so it mirrors the original.

Makes sense to me, can't think of a situation when such logic would break something (although regression reports can be enlightening 😄).

[kleonc]

Since r_dst_rect, r_src_rect are as well inputs in here, it might make sense to return "no draw" also in case any of these rects has zero size.

[lawnjelly]

Although this seems to make sense for r_dst_rect (zero size should presumably not draw), maybe zero size src_rect is valid? The UVs would be the same but it can still draw something. This is similar to the question of whether to not draw in the case of Texture derived classes like GradientTexture. 🤔

Makes me again wonder if we should just mirror the old behaviour for safety.

[kleonc]

Although this seems to make sense for r_dst_rect (zero size should presumably not draw), maybe zero size src_rect is valid? The UVs would be the same but it can still draw something. This is similar to the question of whether to not draw in the case of Texture derived classes like GradientTexture. thinking

Hmm, in theory someone could want to transform vertices in the vertex shader so I guess even in case of a zero-sized r_dst_rect not drawing it could be argued against. 🤔 Not sure whether doing so makes sense / why someone would do it.

But I agree, zero-sized r_src_rect makes more sense. Like someone might want to draw a single color rect by sampling the texture at specific UV (again not sure if doing so makes sense).

So might be safest to not add rect related conditions here.

Makes me again wonder if we should just mirror the old behaviour for safety.

Regarding width/height checks it might be fine to remove them. I'd assume trying to draw kinda invalid (zero-sized) textures is rather not a common case, so not preventing it right here shouldn't affect performance much anyway. And given it's against 3.x it might be better to prioritize avoiding potential regressions here indeed.

[lawnjelly]

Co-authored-by: kleonc kleonc@users.noreply.github.com

Note this doesn't link to my GH account, use 9283098+kleonc@users.noreply.github.com instead.

Ah yes, will change this, wasn't sure how the noreply emails worked. 👍

I like mutable/inout args (performance-wise they make more sense). They seem less readable on the caller side though. Note in #76703 I made args separated based on the Texture::get_region_rect which also passes in/out rects separately. If we're fine with mutable args then I wonder if it would make sense to change Texture::get_region_rect as well to use 2 args passed by non-const reference? thinking

Possibly, but might be better in a separate PR. There's already a couple of things in here that aren't central to the PR (the modulate change and the MeshTexture function removal), and I suspect changing get_region_rect() would be easier to review on its own.

Other thing is I'm not sure about the naming convention for such params. Like what r_ prefix exactly stands for? I've always been thinking about it as "result"/"return". In such case it's kinda confusing / not obvious when such params act also as input. But maybe r_ is meant to mean "reference"? thinking (Doesn't make sense to me though, whether it's passed by (const/non-const) reference is already denoted by the type.)

Afaik Godot just uses r_ for something that may be changed by the function, by reference or pointer. I personally think of it as standing for return but I'm not sure on the origin of the convention.

Also it's even better to copy explicitely only when needed instead of always, possibly for no reason (and hoping compiler will optimize it out).

Yes, this was my thoughts as to eliminating this step.

This version also currently makes no draw the default for all Texture derived classes if width or height is zero. This is currently the behaviour for ImageTexture and a few others, but not for the gradiant textures. I'm just trying to evaluate whether this would be a better all round default / whether it will change anything. If not it's fairly easy to flip the logic here so it mirrors the original.

Makes sense to me, can't think of a situation when such logic would break something (although regression reports can be enlightening smile).

It seems worth a try, rather than complicating the code with adding extra overrides unnecessarily (if there is no need, less code is better). CurveTexture has in built protection to prevent zero size, whereas e.g. GradientTexture does not. There's always the chance this may stop e.g. an unset Gradient from drawing in the editor, but as you say regression reports usually show this kind of thing pretty quickly, and it is easy to change. But equally well am happy to rig it to exactly mirror the old behaviour.

UPDATE: Changed mind on this and it now mirrors the old behaviour.

[lawnjelly]

Based on the discussion for the src_rect, and how a zero area src_rect still might be valid to draw, I changed mind and reverted to mirror the old behaviour, and add the ((w | h) == 0) check only when done by the original code.

[kleonc]

If we want to avoid copying then this should be fine too (under assumption both get_rect_region and refine_rect_region are assigning the results at the end of their execution):

Suggested change

	Rect2 temp_rect = r_dst_rect;
	Rect2 temp_src_rect = r_src_rect;
	if (get_rect_region(temp_rect, temp_src_rect, r_dst_rect, r_src_rect)) {
	return atlas->refine_rect_region(r_dst_rect, r_src_rect);
	}
	if (get_rect_region(r_dst_rect, r_src_rect, r_dst_rect, r_src_rect)) {
	return atlas->refine_rect_region(r_dst_rect, r_src_rect);
	}

I've done it like that originally (pre opening my PR) but changed it for clarity. So if changing like suggested then probably adding some comment would be a good idea.

[lawnjelly]

I used the temps because imo it is dodgy territory using the same variable for both const ref and referenced return argument to the same function, because of aliasing. The function may assume that the const argument is unchanging (either the programmer of the function, or the compiler).

(It may well be UB in fact, I'm having a play in godbolt to see if I can coax this. 🙂 You can immediately see how difficult it would be to optimize around this kind of aliasing.

Ah it looks like the compiler should operate under the assumption that aliasing can occur, which disables some optimizations, unless you use the restrict keyword. So compiler wise this may be ok, but there still is the danger that the function reader / writer / modifier may assume that there is no aliasing, so it's still seems dodgy:

https://developers.redhat.com/blog/2020/06/02/the-joys-and-perils-of-c-and-c-aliasing-part-1#objects_and_types_in_c_and_c__
)

Using the temp enforces the const nature and prevents aliasing, and may well be compiled out if unneeded in this case (as get_rect_region() is known at compile time, even though virtual).

So imo changing this should be left to a future PR that changes the get_rect_region() function.

[lawnjelly]

Rebased (and removed coauthor as now that is already merged).
Should be good now, but needs a second look to check I didn't make any silly mistakes. 😄

[kleonc]

@lawnjelly BTW p_commit_on_flip_change is unused in MultiRect::add:

godot/servers/visual/visual_server_canvas_helper.cpp

Line 160 in 627a290

bool MultiRect::add(const Rect2 &p_rect, const Rect2 &p_src_rect, bool p_commit_on_flip_change) {

It was already done like that in #68960, not sure if you want to fix it in here or in some subsequent PR (this method is unused anyway; but I do see its existence kinda makes sense API-wise).

[lawnjelly]

Ah I'll take a look at the flipping arg, it's a while since I wrote it, it may be historical or for expansion. 👍
Will be for a different PR though.

EDIT: It's probably there for expansion for efficiency, but not fully implemented in the function yet. The efficient API is when you have a bunch of rects to add as efficiently as possible. If the client code knows there are no flips in advance, there's no need to check for flips. But at the moment it checks in all cases.

[akien-mga]

Thanks!