Conversation
bdero
force-pushed
the
bdero/filter-local-transforms
branch
from
17f3cf1 to
c8918ab
Compare
| return std::nullopt; | ||
| } | ||
|
|
||
| auto coverage = inputs[0]->GetCoverage(entity); |
There was a problem hiding this comment.
here and on the guassian filter below: why are we only using the first input? Do we expect there to only ever be one?
It seems confusing to me that this API sometimes takes and uses multiple vector inputs and other times only uses one.
There was a problem hiding this comment.
This funkiness is a side effect of making filter inputs agnostic to the filter implementation (partly inspired by the design of Core Image).
This detail used to be more important because FilterContents would resolve all the filter inputs as textures before calling the render method, but that work is now deferred through FilterInputs. There's still a default coverage routine that most filters won't need to override in practice, but that's been majorly simplified now that everything has a GetCoverage, including FilterInput itself.
Creating Filters in Impeller is locked down to static factory methods in FilterContents that take the intended number of inputs. In the filters themselves, too many inputs isn't an error, but not enough inputs fails the render.
At this point I'm not really married to this detail, but keeping it around might make creating a debug view for filters easier. 🤷 We can probably remove it sometime and make specific SetInputs methods on the filter implementations themselves since it's not really important anymore.
chinmaygarde
left a comment
chinmaygarde
left a comment
There was a problem hiding this comment.
LGTM pending Dan's query. Everything else is nits or can be tackled in a later patch. I really like this approach.
| /// Filters may decide to not evaluate inputs in situations where they won't | ||
| /// contribute to the filter's output texture. | ||
| class FilterInput final { | ||
| class FilterInput { |
There was a problem hiding this comment.
Nit: The headerdoc comments above are now out of date.
|
|
||
| virtual std::optional<Rect> GetCoverage(const Entity& entity) const = 0; | ||
|
|
||
| virtual Matrix GetLocalTransform(const Entity& entity) const; |
There was a problem hiding this comment.
A brief explanation in a headerdoc of the differences between a local transform and the transform would be good.
| if (auto texture = std::get_if<std::shared_ptr<Texture>>(&input_)) { | ||
| return entity.GetPathCoverage(); | ||
| } | ||
| /******************************************************************************* |
There was a problem hiding this comment.
In a later patch if necessary, you could toss these in their own TUs (perhaps in a "filters" directory).
There was a problem hiding this comment.
Ah yeah, I'll do this in a follow-up since #145 changes a bunch of stuff in FilterInput.
entity/entity.h
Outdated
| ~Entity(); | ||
|
|
||
| /// @brief Make a copy of this entity, but with a different transform. | ||
| Entity WithTransform(const Matrix& transform) const; |
There was a problem hiding this comment.
On the fence with this one. Seems like a utility method that provides very little utility. But, your call TBH.
There was a problem hiding this comment.
Eh, I don't have a very strong opinion. Removed it.
bdero
force-pushed
the
bdero/filter-local-transforms
branch
from
e1f636d to
b9ebbde
Compare
3 participants
This is a refactor to add support for local transforms in
FilterContents, which sets us up for adding flexible filter elision behavior andTransformFilters.FilterContents:
RenderFilterandGetFilterCoverage) always receive the entity with the local transform already pre-applied, so that filter implementations don't have to worry about dealing with it. We pass the whole entity around instead of a separate transform because manyContentsuse other entity fields (i.e. the path) for computing coverage. Also,TextureFilterInputuses the path for computing the local transform, which is incorporated when computing coverage.GetFilterCoverage.FilterInput:
FilterInput(as opposed to dispatching with branches based on the variant input). It felt like the right time to do this since we're now up to 3 variants, and each of them are quite unique in their behavior.FilterContentsinput variant which respects the local transform of filters.GetTransform/GetLocalTransformvirtual methods that behave reasonably for each variant.Gaussian image/mask blurs: Use the first input's transform for computing the blur's influence on coverage.