PintaProject / Pinta

Simple GTK# Paint Program
http://www.pinta-project.com/
MIT License
1.81k stars 275 forks source link

Separating API of effect rendering so that there are two stages #808

Closed Lehonti closed 4 months ago

Lehonti commented 4 months ago

Effects are mostly rendered tile by tile, and for each tile they re-compute the same (often computationally expensive) values from EffectData. In order to avoid this, I separated the rendering into several stages, at the API level, so that globally relevant values can be pre-computed first, and then re-used in each tile.

AsyncEffectRenderer should still be modified so that we can reap the benefits of computing the values only once, but in my view this refactoring should be a step in the right direction.

Moreover, the API can still be improved. Note, specifically, that the Voronoi diagram effect still can't pre-compute the values it needs...But if we get the API right, we can get the mentioned effect to be tileable :)

In particular, how would we pass (and use) the ROI data to CreatePreRender? In the case of Render, we just override either the method that gets a single rectangle or multiple ones, but not both, and only the one which gets multiple ones will be called from outside the class. In the case of our new method, CreatePreRender things are different, because that method should be executed only once; so in the meantime (while the AsyncEffectRenderer is modified) I'm making it so that the method only gets the source and the destination surfaces as arguments. After the AsyncEffectRenderer is modified, maybe we could also pass it, all at once, the collection of all ROIs so that the data in the first stage only get computed once.

Finally, I'd suggest that the label "API" is created for issues and pulk requests like these.

cameronwhite commented 4 months ago

Thanks! I'll need to spend a bit more time looking at this and thinking through the issues you mentioned I've added the api tag though

cameronwhite commented 4 months ago

For the notes about the limitations for the Voronoi diagram effect - would that effect need to know the entire list of rectangles being rendered, or just what the overall bounding rectangle is?

One other idea would be to instead just pass the whole overall region into the effect and handle any parallelism inside the effect, rather than how the caller handles that currently. It could have some API to report progress back to the live preview renderer if certain tiles have finished rendering. Any thoughts about that approach? It at least would avoid making the caller deal with this extra "pre-render" step to create opaque data that's only used by the next method call

Lehonti commented 4 months ago

@cameronwhite I think it's better if it knows the overall bounding rectangle. In this case, the issue is that AsyncEffectRenderer and related code would have to be heavily modified, I think.

As for the second paragraph of your response, it would be a good idea, but again it would require heavy modifications. What about this signature? What would you add or improve?

Render (
    StatusListener status,
    ImageSurface source,
    ImageSurface destination,
    RectangleI imageBounds,
    RectangleI effectBounds);
cameronwhite commented 4 months ago

I think the image bounds could be computed implicitly since that's just the source image's width and height? Otherwise that signature is roughly what I had in mind

Lehonti commented 4 months ago

Yeah, that makes sense.

In order to do this, the AsyncEffectRenderer has to be refactored, which is not that trivial, but it can be done given enough time.

cameronwhite commented 4 months ago

Yeah, it is a larger rewrite of the live effect preview, so this might be change to wait until after the next release for

Lehonti commented 4 months ago

In that case, I'll close this so that the list of pull requests is kept clean, and I'll open a new one when the AsyncEffectRenderer has been properly refactored or re-written