Document Image.blend_rect() blends sequentially per-pixel

[Arecher]

• Added a note to Image.blend_rect() to explain it does not blend the two images in one go, but reads and writes each pixel in sequence.

Additional information

I decide to add a linebreak to avoid the block of text becoming too large and unwieldy.

Open Questions

I presume blit_rect() and the .._mask() functions work the exact same way. Adding this note to all of them felt a bit overkill, but would be correct. I'm unsure what the best solution is here, so I kept it at just blend_rect() for now.

I considered adding a starting from the topleft as a way to explain how the sequential per-pixel blending is ordered, but I decided that might be overkill and unlikely to be incorrectly assumed.

Originally I was planning to note that using Resource.duplicate() on the read image is a way to work around this issue, but I decided not to. This could be added again if that's preferable.

When working on this, I noticed that the input argument names are unusually unreadable for Godot. Is there a reason it is referenced as src rather than source(_image)? And what does dst even stand for? Distance? Isn't it simply a pixel coordinate that the rect should be pasted at? I'm unsure what the conventions for this are, and whether making changes is compatibility breaking, but I feel like this is a potential area for improvement.

Reason for change

My expectation for blend_rect was that it blended the entire rect of the image onto the target image in one go. This is, for example, how Aseprite's Image:drawImage(image, rect, ...) works (at least in how it is presented to the user, internally I'm sure it works sequentially per-pixel too). But since in Godot it reads and writes each pixel one by one, the write from a previous pixel can impact the read from another, causing odd artifacts. This makes sense in hindsight, but caused more confusion than necessary at the time. The fact that my mental model of how the function worked and how it works in reality were so far apart, warranted a note to better align the two for any future readers.

I understand that the real issue stems from the fact that I didn't immediately remember that image references in Godot are immediately updated and not generally automatically duplicated when used in functions, and that with that information, the blend_rect() function behaves exactly as expected. But since users that already have that innate understanding likely won't run into this issue, I instead chose to explain this in a 'per pixel' versus 'in one go' approach. After all, if you think about this not as a singular action, but as each pixel being updated one-by-one, it makes sense that the next pixel can be influenced by pixels earlier in the loop. And with that, this addition should hopefully catch only the users that have the wrong expectation, while not getting in the way of those that do not.

[Arecher]

While it does not close this issue #121499 (comment), it does resolve the problem I encountered in this issue, and my desire for improved documentation that came out of that.

[skyace65]

Are you working on other projects that require PR descriptions this long for small documentation changes like this?

[Arecher]

Apologies that they are unusually longwinded! I was simply trying to cover my bases and explain why I think these changes have merit. I'm mostly used to the Proposals repository, where making a strong case for why this is important feels mandatory, otherwise small quality-of-life changes (similar to these documentation notes in my opinion) are quickly disregarded as unnecessary bloat.

If that isn't important for documentation changes, I'll definitely keep them shorter next time!

[skyace65]

@Arecher I'm a bit confused here. The change we merged from you yesterday in the documentation repository had a brief description (which was good) which nobody gave you any issues for, but I'm also seeing the same kind of long descriptions in your other documentation repository PRs that were opened after that.

[Arecher]

@skyace65 I wasn't referring to any trouble specifically, especially not recently. But generally over the years with occasional proposals or issues, I just found that you really need to make a strong case for why this is relevant or a good addition.

My previous documentation change was a simple outdated variable name, which is something that everyone can agree on needs to be changed, so there was no need to 'justify' anything. With (some) of the documentation changes I made yesterday, it felt like there would be a higher chance users (especially those that are super familiar with how the engine works), believing that the additions are an over-explanation or a non-issue. In other cases there were certain uncertainties about the clarity of my explanations, which I thought were worth pointing to, so they could be double-checked or added to by someone with a more thorough understanding. I'll try to only update documentation when I'm pretty certain of how something works, but I don't have the behind-the-scenes knowledge that many of the maintainers and other contributors do. On top of that I guess I wanted to be careful and thorough, since these are my first contributions and I'm not entirely familiar yet with the general practices!

[skyace65]

@Arecher I understand. Generally speaking for documentation only changes, and this applies to the doc repository as well as the engine one, you don't need to do explanations this in depth. Just give a brief overview over what you're changing and why. Again, what you had in the documentation repository PR was perfect. Longer descriptions can add to review time, and if anyone has any questions they will let you know during review. We don't close documentation PRs because we're confused by something without first asking the author of the PR for clarification.

[AThousandShips]

Looks good from a style and form perspective

[AThousandShips]

Suggested change

	Simultaneously reading and writing to the same image reference may lead to unexpected results.
	[b]Note:[/b] Simultaneously reading and writing to the same image reference may lead to unexpected results.

[Repiteo]

After applying ATS's suggestion, could you squash your commits? See our pull request guidelines for more information