Add Property bag to Configuration and Image Processing Context #1176
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1176 +/- ##
==========================================
+ Coverage 82.47% 82.49% +0.02%
==========================================
Files 690 691 +1
Lines 29918 29941 +23
Branches 3380 3382 +2
==========================================
+ Hits 24674 24699 +25
+ Misses 4542 4540 -2
Partials 702 702
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
We should decide on a several design questions before continuing:
- The new concept somewhat overlaps with the way we are using
Configuration. When to prefer values in the property bag? When to prefer strongly-typed properties onConfiguration? - According to decisions on 1: are there any existing API-s we should change for the sake of consistency in the core library?
- Not urgent, since we can wait with API stability, but: Same question for
ImageSharp.Drawing. I think this enables composition over inheritance techniques: Have a separateGraphicsOptionsandTextOptionsclasses Instead ofTextGraphicsOptions : GraphicsOptionsinheritance. - Shouldn't
IImageProcessingContext.Propertiesinherit all values ofConfiguration.Propertiesby default?
|
I was hoping this would trigger a discussion about this.
I can see there being some use cases around encoder and decoder options that might make use of this.
Only thing to immediately consider is do we remove the overloads that take a
The way I setup the |
But in that case you need to do the same steps manually for other properties, I'd rather prefer to solve this by design, if possible.
Can we have a quick consumer-side API comparison? How does a sample drawing mutation call using |
Currently, we configure encoders/decoders with |
img.Mutate(c => c
.Vignette(new GraphicsOptions
{
AlphaCompositionMode = PixelAlphaCompositionMode.Clear,
ColorBlendingMode = PixelColorBlendingMode.Darken
}, Color.Red)
.Vignette(new GraphicsOptions
{
AlphaCompositionMode = PixelAlphaCompositionMode.Clear,
ColorBlendingMode = PixelColorBlendingMode.Lighten
}, Color.Green)
.Vignette(new GraphicsOptions
{
AlphaCompositionMode = PixelAlphaCompositionMode.Clear,
ColorBlendingMode = PixelColorBlendingMode.Multiply
}, Color.Blue)
);vs using SetGraphicsOptions img.Mutate(c => c
.SetGraphicsOptions(new GraphicsOptions
{
AlphaCompositionMode = PixelAlphaCompositionMode.Clear,
ColorBlendingMode = PixelColorBlendingMode.Darken
})
.Vignette(Color.Red)
.SetGraphicsOptions(o => o.ColorBlendingMode = PixelColorBlendingMode.Lighten)
.Vignette(Color.Green)
.SetGraphicsOptions(o => o.ColorBlendingMode = PixelColorBlendingMode.Multiply) // alpha still set to Clear
.Vignette(Color.Blue)
); |
|
I need to have a proper think about this. I'm not sure why |
|
don't worry about it too much... it was just a thought really, we'll leave the overloads on that take an options too. We definitely need/want the extensibility to load defaults and pass global parameters otherwise Drawing can't add global config options. |
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
Ok. So I like the idea of an extensible property bag in the configuration since that lets us extend Configuration without breaking the API.
I don't think we need the additional bag in the context though. If we want something global it I believe it should be stored in Configuration instances to prevent ambiguity. Individual processors can accept options as parameters, falling back to the Configuration property bag and extension methods when not available.
|
I really want to keep the version on context too as it opens up a world of options.. I will want to leverage it more on the drawing side and wanted to get it in now. |
I don't understand the use case. The |
|
Its the same difference as there being a Headers collection no Main use case is on processing SVGs where we want to be able to record and apply transforms and masks on parts of a processing tree and then revert back later without having to pass loads of complex options around..... not everything in the tree will even need to know about the setting from higher up it would just work and compose into the correct setting. We shouldn't really be encouraging people to create new configuration instances all the time everything should just compose and work. Your point of reference for the API at the moment is based on the straight processor base manipulations that are mostly independent of each other with Drawing they are all variants and interaction on the same underlying processor, the way you want to use the API is different... You don't want to have to call a transform on every single vector on the way thru every time you want to be able to do something like |
|
using (var image = new Image<Rgba32>(100, 100))
{
var contextConfig = new Configuration();
image.Mutate(
contextConfig,
x => x.Hue(180)
.Pixelate(4)
.Saturate(.75F)
.Vignette());
}So I see now where you are coming from now API-wise. It's definitely nice to set the options via an action than by splitting up or constructing individual options. Do we need the |
Yeah we do. Without the |
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
Yep, you've convinced me. This is a useful and powerful addition.
antonfirsov
left a comment
antonfirsov
left a comment
There was a problem hiding this comment.
Parameter naming + some further points to consider.
| /// Gets a set of properties for the Image Processing Context. | ||
| /// </summary> | ||
| /// <remarks>This can be used for storing global settings and defaults to be accessable to processors.</remarks> | ||
| IDictionary<object, object> Properties { get; } |
There was a problem hiding this comment.
I wonder if we want to consider something more strongly-typed here?
System.Typeor a custom object as a key- Restrictions for the value: do we expect simple values here? If not, maybe a marker interface.
- Our own types
GraphicsProperties/IGraphicsPropertiesinstead ofIDictionary<K,V>
There was a problem hiding this comment.
no it should be explicitly be as loosely typed as possible and we should not be limiting anything from being but in the property bag, that's why I even when as far as using <object,object> rather than <string,object> to allow developers to use it for there needs they can do anything with it... use strings, use private object instances (no way for someone to accidently reuse the key) or anything in between... we would be loosing one of the key flexibility benefits of this extension point is we make the type a custom type with restrictions..
| return go; | ||
| } | ||
|
|
||
| var configOptions = context.Configuration.GetGraphicsOptions(); |
There was a problem hiding this comment.
Are we really sure we want to repeat the defaulting logic in all the Get*** extension methods, instead of populating Properties in the DefaultImageProcessingContext ctr?
What are the pros for this decision?
There was a problem hiding this comment.
Ah yeah... If our keys are strongly typed that makes comparison faster and easier too.
There was a problem hiding this comment.
FYI an interesting API proposal that also contains the concept of property bags: dotnet/runtime#1793
In total, a bit too verbose for my style, but I like the idea of having custom types for the bag, and (in some form) for the keys. I would also apply it for values, if it makes sense.
There was a problem hiding this comment.
I do kinda like the pattern proposed in the runtime but, (and it is a big but) it feels soo overengineered, sometimes (and in my experience usually) the simple solution is the right one.
Prerequisites
Description
This adds a property bag to both the
Configurationclass and theIImageProcessingContext<T>interface to allow for passing defaults and any other contextual information between calls against the context. Includes an initial implementation forGraphicOptionsclass to allow setting global blending options etc across the entire application or just the mutation.This will likely play more into drawing in the future where we will want to be able to define global fallback fonts (enabling default emoji fonts etc), anti-aliasing fill modes etc.