-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
.NET core should have primitive drawing types #14503
Comments
Method argument names need some tweaking... Some examples: - public Point(Size sz) : this() { }
+ public Point(Size size) : this() { } - public void Offset(Point p) { }
+ public void Offset(Point point) { } - public bool Equals(Point pt) { return default(bool); }
+ public bool Equals(Point other) { return default(bool); } - public static bool operator ==(Size sz1, Size sz2) { return default(bool); }
+ public static bool operator ==(Size left, Size right) { return default(bool); } There are a bunch of other similar cases; same feedback applies throughout. |
@justinvp : This is just a speclet, i'll fix these names in the actual PR, that will be committed. |
Integral Point/Size/Rect are quite useful as well. Should we make these generic or just create float/int variants as system.drawing does? |
We might want to qualify Color a bit more, say, by calling it ColorRgba. If we're trying to represent an RGB value, in byte form, then we need to specify the color profile (or list it as an assumption). sRGB is similar to gamma=2.2, and both avoid the visible banding that occurs if you try to store linear RGB values in 24 bits. If we accept floating-point parameters, we need to be clear about whether they are in linear form or 'compressed' sRGB form. Typically floating point RGB values are assumed to be in linear light. Since composition and averaging of color values must (for correctness) occur in linear light, we should probably expose a float variant if we want to offer related math functions. Also, if we want to reuse this for (fast) interop, we would want the structure to have byte offsets such that it can be stored as a 32-bit integer in BGRA form (little-endian, windows), or ARGB (big-endian, ARM, etc, linux). Things get much slower if there is a memory layout discrepancy when compared to the host platform. Regardless of interop, memory layout matters. Anything deal with large arrays of colors will be very sensitive to cache misses, which will happen much more frequently if we're taking 128 bytes (4 fields) instead of 32. Punting and mirroring the CSS color specification, listing Color variants as storage-only representations might work. Although there are sure to be lots of extension methods that deal with them incorrectly. I'd suggest making Color a separate specification, as it has more aspects to consider than Point/Size/Rect. |
I agree with @nathanaeljones. Color needs to be separate and flexible enough to deal with various color spaces. I wrote some code a while back to deal with conversions. See https://github.com/leppie/Colorspace, if interested. |
I think the Color data type should stay as it is proposed. If chosen to take the other path there could be countless other Color types that all have different internal structuring. I don't think .NET Core needs this and is very application specific. Int based Point/Size/Rectangle could be beneficial however but don't see it being that important. I mean if the choice is made to go slightly out of scope of .NET Core and define every single drawing primitive type possible then why not, but I think its best to keep it simple. |
The Color type is still based on RGB so it's easy to add more methods. public static Color Invert(Color color) { return default(Color); } // Inversion of alpha is not necessary
// Maybe:
public static Color Light(Color color, float amount) { return default(Color); } // lerp towards white
public static Color Dark(Color color, float amount) { return default(Color); } // lerp towards black The API Proposal decribes the methods GetBrightness, GetHue and GetSaturation - a one-way conversion from RGB to HSB. A two-way conversion would be great and consistent. This would cover simple use cases for RGB and HSB without the need for special Color types. Are there thoughts to add a method to create a color from r, g, b? |
@xanather Could you be a bit more specific about the use-cases and the context you're making these statements from? |
@patricksadowski The point is that Light and Dark have completely different meanings depending on the color space you're talking about. And do you want 'amount' to be perceptually linear, as would be intuitive? Or mimic some gamma curve, as the naive implementation would? |
I have a few questions: Point.IsEmpty // what does it do? Why isn't point a generic type and this class one of its implementations ( public static Point Ceiling(Point value) { return default(Point); }
public static Point Truncate(Point value) { return default(Point); }
public static Point Round(Point value) { return default(Point); } I don't think we should have that operations on point like that (and other similar), I'd move them to separate class with static extensions. Point is not something you do mathematical operations on, that's what vector is for. Point is just a data structure and should not represent a two element vector. Same comments for Rectangle and Size. // I don't understand what do these method names mean but looking at args:
// Why not FromRGBA?
public static Color FromNonPremultiplied(int r, int g, int b, int a) { return default(Color); }
// Why not: FromVector4, also feels like FromVector3 is missing
public static Color FromNonPremultiplied(Vector4 vector) { return default(Color); }
// Why not Interpolate?
public static Color Lerp(Color value1, Color value2, float amount) { return default(Color); }
// No clue what does this do but whatever graphical represantation you can see you should be using Vector4 for computation and Color for representation
public static Color Multiply(Color value, float scale) { return default(Color); Also I believe we should completely remove any dependencies on Vector and rather add some static extensions class to make conversions. Why might factor them out to separate assembly in the future. |
Point.IsEmpty // what does it do? This checks for if point is empty (0,0) public static Point Ceiling(Point value) { return default(Point); }
public static Point Truncate(Point value) { return default(Point); }
public static Point Round(Point value) { return default(Point); } These APIs were taken from existing System.Drawing.Point, and these are defined as methods over Point. Following already existing design, and I don't agree with Point is not something you do mathematical operations on, it's a mathematical data structure. Vector has a magnitude and direction, Point is just a location in space. And these methods are not vector methods. // I don't understand what do these method names mean but looking at args:
// Why not FromRGBA?
public static Color FromNonPremultiplied(int r, int g, int b, int a) { return default(Color); }
// Why not: FromVector4, also feels like FromVector3 is missing
public static Color FromNonPremultiplied(Vector4 vector) { return default(Color); }
// Why not Interpolate?
public static Color Lerp(Color value1, Color value2, float amount) { return default(Color); }
// No clue what does this do but whatever graphical represantation you can see you should be using Vector4 for computation and Color for representation
public static Color Multiply(Color value, float scale) { return default(Color);
This method is not to construct a rgba color, it converts a non-premultipled alpha color to a color that contains premultiplied alpha. Refer docs on what's premultiplied alpha
Same, these methods are to support alpha composting, it requires an A value, so no Vector3.
Lerp = linear interpolation, this is the quasi acronym for linear interpolation and widely used in other frameworks like unity, etc.
This method can be used to lighten or darken a color by a given scale. Refer stackoverflow [http://stackoverflow.com/questions/12894605/how-to-make-specific-color-darken-or-lighten-based-on-value-in-wpf] |
@nathanaeljones @xanather : On adding int based types, based on analysis done by Experience and Insights, they said integer versions were not widely used, and can be added later if there are limitations without the int type. We want to keep the Core implementation concise. |
The color constructors can be used to create a color from r,g,b or vector3, vector4. |
It wasn't exactly that the integer versions of Point, Size, and Rectangle aren't widely used. The System.Drawing drawing APIs support them so people naturally do use them. Rather, we aren't sure whether integer versions of these types are necessary. Looking at more recent APIs than System.Drawing, we saw that out of WPF, WinRT, OxyPlot, and NGraphics, none of them had integer versions of these types. A single type to represent a Point is much less cumbersome than trying to find good names to disambiguate (ie Point/PointF), or using a generic type like We're definitely interested in feedback on this. Are there scenarios where the float versions of the types wouldn't work well? |
The proposed Color type is still based on (s)RGB so the color space is fixed. As proposed Light and Dark use Lerp so the meaning of 'amount' should be defined there. @Priya91 |
IMO, it makes sense for a |
👍 for @MrJul's idea of having |
Should these types really be mutable? |
No, I'm hoping that was a typo. Mutable structs are a footgun. |
For information,
Since we're still in the graphics domain, it's probably done on purpose. |
Well, early versions of ImageResizer used floating point exclusively, only rounding at the System.Drawing API boundary. It turns out that this is an easy way to introduce floating-point bugs. Storing semantically integer data as floating point is much more problematic than it would seem, and we had more bugs than lines of code. Floating point is a bit more difficult to reason about than integers. You have non-comparable values like NaN and +/- infinity. Good luck trying to implement rounding or comparison that is consistent with an external language or library. Debuggers are also the devil with floating point, as they will round values for display, hiding the .99999999999 that is actually in memory. I'm concerned that providing only floating-point variants in Core will reduce the correctness of software and APIs that want to use Point/Size/Rect. |
Tuple is immutable. I'd wager it is more widely used than Vector2f. |
Related: with floating point, how close do X and Y need to be to zero for |
@MrJul @jasonwilliams200OK : Origin is (0,0) only in cartesian coordinate system, otherwise origin may be chosen as any point of reference. |
@Priya91 Forgive my ignorance, but doesn't the Point type represent a point in Cartesian space? What other coordinate system could it represent? |
@dsplaisted I was thinking along the lines of origin being relative, as in my point of reference (x',y') can be different from (0,0), the point where x axis and y axis meet on the cartesian plane. I am not sure if the concept of origin in graphics always remains (0,0), or changes with the images modeled. I am fine with renaming Empty to Origin, and leave the context to the consumers of the library. |
Thanks for the explanation @Priya91.
Other options are:
|
I also support the suggestion of @ebickle to work towards a legacy free geometry library, using the existing Vector2/3 types where appropriate. Would https://github.com/dotnet/corefxlab be a location where such work can be hosted while under development? It would be nice to have an area where high churn, discussion and experimentation is acceptable, but which also indicates this is work intended towards incorporation into corefx in future |
Yes, I think so. It wasn't stated initially, but one of the core scenarios that drove the creation of this work item is discussions with a fair number of (non-graphics) customers (e.g. financial) that have existing .NET applications that use these drawing types, just because they were convenient and approximately what they needed for their app. The lack of these basic types in the .NET Core surface area makes it that much more challenging for them to migrate to .NET Core, which they want to do for a variety of reasons, including cross-platform support. Making these basic primitives available largely as-is is then in my mind really about enabling these customers to move over, such that more existing code continues to "just work." It's not focused on real graphics workloads, for which such primitives are in-and-of-themselves insufficient. My suggestion is two-fold:
I think we should re-focus this issue on (1), to make the existing primitives available; what exact existing types do we want to bring in, what do we want to call the library, etc.. Then we'll have a separate project/set of issues on corefxlabs for making progress on (2). Reasonable? |
👍 now that the core scenario for the creation if these types has been explained, it's a bit clearer to me. |
@stephentoub, corefxlab sounds like a fine place for dotnet/corefx#2. I would be more than happy to accept a PR that adds such library to corefxlab. |
Sounds ok to me, as long as this is made undoubtedly clear in the description of the package and everywhere else. |
Basically the only issue to discuss is then the assembly name (the code can presumably be taken from the .NET references source). I suggest But the motivation for adding these compatibility types sounds like a slippery slope to me. Would porting to corefx not likely go well beyond six tiny struct types that are missing? And do the missing types have to be part of corefx, or would any compatible assembly on NuGet also satisfy the porting use-case? I use the WPF types While the proposed set of types are small and the types simple, this issue and the interest in it highlights a lot of confusion around the corefx positioning. |
I've added an issue to corefxlab for further discussion of the legacy-free direction: dotnet/corefxlab#86 |
I have some concerns about porting the existing System.Drawing primitive APIs to .NET Core as "legacy" with the intent of creating new legacy-free APIs. The existing types are "good enough". So even if we mark them as legacy or obsolete, people will still use them for non-legacy code. This is especially true since porting the existing APIs is relatively easy so we can do that fairly quickly, while designing legacy-free APIs will take longer. So there will be a time when the legacy APIs are the only ones available, and the ecosystem will start to adopt them. I also think that even though there are pitfalls with mutable value types, there may be people who find mutable APIs easier to use and will thus prefer the legacy APIs. Basically I think we risk a split where the libraries in the ecosystem don't agree on a set of exchange types for these concepts, hurting interoperability. I'd like more clarity on what the "legacy-free" APIs would look like before we move forward with porting the existing APIs as is. That would help us make a more informed decision. A few other points:
|
I have some tips; is there a place for related design discussions? |
There will be. It will probably be another GitHub issue. We will definitely be interested in feedback and expertise from you and the rest of the community. |
Wow, it is a long thread with lots of in-depth discussion! I will try to give some feedback from a Point/Rectangle for integers
At the very beginning of SharpDX, these structs were used directly by using PointF vs Vector2Mathematically the semantic of a point is quite different from a vector. A Size (Size2. Size2F, Size3, Size3F)This one has also a specific semantic Instead of plain
ImmutabilityFor the types discussed here (Size, Point, Plane...etc.), it is not much an issue having immutable types. They are barely modified in intensive loops. It is more a problem when you force immutability on types that are bigger and can be modified in batch on a particular component and you want to avoid the whole recreation of a struct (causing a bunch of copy on the stack, copy back on the heap...etc.) It it less an issue for small structs - 8 bytes-, it becomes a bit problematic for 16 bytes (Vector4...), is is absolutely a non-sense for large structs >= 64 bytes (Matrix...). Suppose you just want to apply a translation on a single axis to a matrix, you don't want to copy-back-and-forth 64 bytes just to modify 4 bytes. For a common .NET developer, this discussion could sound a bit finicky, but when you start having a budget constraint of 16ms (60 FPS in games), you really care about these details. Colors vs Geometry or GraphicsI agree that Color needs special crafting. There is a need to correctly handle:
I don't have any particular incentive at keeping backward compatibility with Though I would not mix Texture/Image manipulation with |
I agree with @stephentoub and @weshaggard to bring forth the existing drawing types from System.Drawing. These types are intended to be independent of any graphic stack (we have a separate issue tracking developing a geometry library for image processing scenarios dotnet\corefxlab#86). The only concern is with Color, existing color has a lot of knowncolor values, which gets rendered differently based on the graphics adapter, hence skipping these on .NET Core. For the other vector operations on color, like lerp, scaling, etc, we can define extension methods. Note that the proposed API does not have any new additions on Point, Size and Rectangle that is not there in the existing drawing types. If everyone agrees with bringing the existing types, Issues to discuss:
|
|
I write lots of code for generative art and I modify colours, polygon vertices (points) and vectors all the time. If the structures were immutable I'd have to create tens or even hundreds of thousands of new structures per frame, potentially millions per second. e.g. foreach (int element in bigarraywith10000000elements)
{
var c = element.colour;
// just how stupid does the next line look
var newc = Color.FromArgb(c.A+1, c.R, c.G, c.B);
element.colour = newc;
} Of course I could have made it look in worse by doing element.colour = Color.FromArgb(element.colour.A+1, element.colour.R, element.colour.G, element.colour.B); just to reinforce that I'm having to create a new struct to increment the alpha of the existing one foreach (int element in bigarraywith10000000elements)
{
element.colour.A += 1;
} You can imagine which would perform better too. |
From the discussion, it is clear we need int and float based versions of the primitives, and comparing the proposed and already existing system.drawing types, there is not any difference in the api surface area except for the name changes. Hence it makes sense to expose the already existing System.Drawing primitives as is, that is, Point, PointF, Size, SizeF, Rectangle and RectangleF to .NET Core. We are not exposing Color at the moment because we feel the existing Color type in System.Drawing is insufficient as specified in the above conversations. Once the existing types are exposed, we will start a separate discussion on crafting Color primitive for .NET Core. |
@richlander : Yup, SharpDX has Point, Rectangle and RectangleF. There is no float version of Point. Point also has implicit and explicit conversions to Vector2, which we can provide via extension methods over Vector2 and Point in a separate Extensions contract as ToVector2(this point p) and ToPoint(this Vector2). The APIs are also similar between System.Drawing and SharpDX. |
We've reviewed this week and it looks good as proposed. Notes are available here. |
Is there somewhere where we can get the code while waiting for it to be included? What about System.Drawing.Font, is this planned too? |
Will these types still use the hardware acceleration (SIMD) underneath (where applicible), or do we need to use System.Numerics.Vectors directly if we want that? |
Has this been canceled? dotnet/corefxlab#86 |
Just wanted to add that a class like "Rectangle" should have properties TopLeft, TopRight & BottomLeft, BottomRight. Also, instead of having a standalone-class like Rectangle, Triangle, Trapezoid, Square, etc. it would be nicer to have a single more general/abstract class like GeneralizedNonOverlappingPolygonWithNCorners instead, with properties Midpoint, Barycenter , area and circumference. These aren't hard to code. Something more like this:
Implementation (ActionScript):
|
Issue: #14431
Based on the analysis done by Experience and Insights, primitive drawing types are commonly used even outside Image processing scenarios. The following types are to be considered:
From the discussion, it is clear we need int and float based versions of the primitives, we decided to expose the already existing System.Drawing primitives as is, that is, Point, PointF, Size, SizeF, Rectangle and RectangleF to .NET Core. We are not exposing Color at the moment because we feel the existing Color type in System.Drawing is insufficient as specified in the below conversations. Once the existing types are exposed, we will start a separate discussion on crafting Color primitive for .NET Core.
Contract
System.Drawing.Primitives
API Proposal
The text was updated successfully, but these errors were encountered: