Skip to content

Refactoring CloudsEffect to compute each pixel individually#1695

Merged
cameronwhite merged 5 commits into
PintaProject:masterfrom
Lehonti:refactoring/clouds_pixelwise
Aug 30, 2025
Merged

Refactoring CloudsEffect to compute each pixel individually#1695
cameronwhite merged 5 commits into
PintaProject:masterfrom
Lehonti:refactoring/clouds_pixelwise

Conversation

@Lehonti

@Lehonti Lehonti commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

The aim of this is making the Render method concise and get the color of each pixel individually, like in many other effects.

However, there is a caveat. The current implementation (before this pull request) uses the blend-ops of Cairo surfaces, which turn out to be different from Pinta's in some cases.

One possible solution would be implementing the remaining blend-ops. Another possible solution would be removing them from the effect itself and letting the user apply them by using layers and changing the blend mode of the layer.

@Lehonti Lehonti marked this pull request as draft August 26, 2025 09:09
private static ColorBgra GetFinalPixelColor (in CloudsSettings settings, ColorBgra original, PointI coordinates)
{
int dx = 2 * coordinates.X - settings.Roi.Width;
int dy = 2 * coordinates.Y - settings.Roi.Height;

@Lehonti Lehonti Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation note:

This one uses settings.Roi as the offset, the color of a pixel depends on its position with respect to the render bounds.

If we want it to depend on the absolute coordinates, that is, with respect to the canvas itself, independent of the render bounds, we need to replace this with settings.Size, and update the tests

@Lehonti Lehonti changed the title Refactoring CloudsEffect Refactoring CloudsEffect to compute each pixel individually Aug 27, 2025
@Lehonti Lehonti marked this pull request as ready for review August 29, 2025 19:37
@Lehonti

Lehonti commented Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

In the last commit I removed the blending operations for the reasons described in previous messages, and given the insights obtained working on #1700, #1696, and #1553. I think we might (but don't know if we necessarily should) be able to re-implement them once we re-write the existing blend ops so that they work with premultiplied alpha.

@cameronwhite

Copy link
Copy Markdown
Member

I'm not sure if I'm clear on what the the reasons described in previous messages are since there are a lot of open PRs related to this.
Are you thinking that it's a better workflow to have users create a new layer and configure its blend mode? Otherwise I don't think we should be removing a feature just to clean up the code, and instead could wait to have the UserBlendOps be fixed and use them here

@Lehonti

Lehonti commented Aug 30, 2025

Copy link
Copy Markdown
Contributor Author

@cameronwhite I'm not necessarily saying it's a better workflow (although it could be, because the functionality for choosing a blend mode is already implemented for the layer mechanism in Pinta so we would be avoiding repeating functionality).

This CloudsEffect is currently using the blending operations for Cairo surfaces in order to do the blending.

However, the UserBlendOps we have for individual pixels are a different set (and, of course, as we've seen, don't seem to work for premultiplied alpha). So currently the only way to do the blending pixel-by-pixel and offer the same functionality is having the user do it in two separate steps.

If we wait for the UserBlendOps, and we want to offer the same functionality as with the layers, we wouldn't just need to fix the existing blend ops, but we would also need to create the missing ones. From the diff for 11a128c, we can see that those are:

  • Hard light
  • Soft light
  • Color
  • Luminosity
  • Hue
  • Saturation

It is possible that there might be one or two that are the same as the ones Cairo offers, just with a different name, and I would have to check that; but I think it is very likely that they are just missing.

@cameronwhite

Copy link
Copy Markdown
Member

Thinking about this more I'm okay with removing it. Creating a new layer and configuring the blend mode is reasonable, especially since you might also be adjusting the opacity at the same time too, and none of the other "generator" effects like the fractals have the same option

@cameronwhite cameronwhite merged commit e0543c7 into PintaProject:master Aug 30, 2025
6 checks passed
@Lehonti Lehonti deleted the refactoring/clouds_pixelwise branch August 30, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants