cpp-io2d/p0267_refimpl

Reference Implementations of P0267, the proposed 2D graphics API for ISO C++

p0267_refimpl's Issues

Surface constructors are inconsistent

I have been experimenting with this library and have a question about the following constructors.

image_surface image{io2d::format::argb32, 640, 480};
display_surface display_surface{ 640, 480, io2d::format::argb32 }

Is there a reason why the argument order for these two constructors are not the consistent?

Many thanks for work on this library and the standards proposal.

Divide header by paper topic

That is to say:
Synopsis
Colors
Linear algebra
Geometry
Text
Paths
Brushes
Surfaces
Input
Standalone functions

Message queue callbacks

Taking charge of the message queue isn't going to fly: display_surface::begin_show and display_surface::end_show need to be joined by e.g. display_surface::update(time). The time parameter should be the time to advance by, defaulting to zero to allow the library to infer the elapsed time.

Ultimately, display_surface needs an additional constructor to pass a client window class. Perhaps the thing to do is to define an ABC for client windows and provide a default, although I am queasy about the idea of using such a technique for something known at compile time. Nonetheless, it's a starting point.

Consider using one or more other libraries for PNG and JPG support

Perhaps GraphicsMagick?

Consider adding additional formats

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

The format enum is currently a bit sparse. We should consider adding additional formats for pixel data beyond the handful that exist in the format enum. The suggestion links this with issue #12 (it's given a separate listing here because I think it's worth considering regardless of how issue #12 is decided).

image_surface type fails to properly preserve its data

Creating an image_surface from data requires the creator to maintain the data until the surface is destroyed. Right now the implementation does not do this.

Verify that void* closure parameters are useless and then eliminate them.

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

Generalized from the suggestion regarding the image_surface ctor that derives from cairo_image_surface_create_from_png_stream.

Stateful drawing APIs considered harmful

This proposal is copying the stateful surface interfaces found in 2D canvas libraries of yester-year. We've (re)learned a lot of lessons since then as an industry and now consider that kind of implicit state stack to be harmful.

The largest issue is that the implicit state strongly hampers composability. Consider this fragment:

surface s;
s.set_color(red);

some_function(s);
// wait, did I have to save()/restore() around that ?
// does it break if I don't ? does it perform poorly if I do ?

s.fill(rect);

Function composition is severely harmed as users now have to do extra work to track the state that surface is bundled with, i.e. they are tracking the tracking of the state. The users have to manually deal with implementation concerns of functions they call. If the user just plays it safe and calls save/restore around any such function then they can end up with degraded efficiency (assuming they never make a mistake and forget, which real users certainly will sometimes!).

Modern graphics APIs have all moved strongly away from implicit state. In the 3D space we've seen misguided OpenGL model superceded by stateless Vulkan, and before that saw its lunch eaten by DirectX's stateless model. On 2D, the under-performing Cairo library that this paper draw inspiration from has been superceded by Skia or by Azure/Moz2D.

Note both of the linked APIs encode state as a separate object passed to the paint operations directly. There is no implicit state tracking in the API. A user function can only alter state if its interface explicitly takes the state as a non-const reference type, exactly as we would expect of a thoughtfully designed and mature API.

I very highly recommend reading through the design rationale of Skia or Moz2D. Cairo and Direct2D are the wrong models to base a new drawing library upon, especially one that we might want forever embedded into the C++ standard library. Stateful interfaces are bad for code composability, bad for performance, and bad for modern low-level hardware APIs.

Ensure R5 compliance

However, add/keep the fixes from cpp-io2d/io2dts#49 since those should be uncontroversial and I'd rather people use the R6 function names since the R5 names were typos that weren't fixed before R5 was submitted.

Remove the cairo PNG functionality

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

path_builder::get_current_point() wrong

The implementation of path_builder::get_current_point() is wrong since it dereferences _Data.crend().

Possible fix:
Simply return _Current_point which already contains the information requested:
point path_builder::get_current_point() {
assert(has_current_point());
return _Current_point;
}

Cheers

About window does not show up in Win32 version

Clicking on the About menu item in the Help menu does nothing. It should show a dialog.

Expand/change font_slant and font_weight

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

The suggestion is to add more values to font_weight and consider letting the user specify percentages for both font_weight and font_slant.

Why are std::function objects wrapped in unique_ptr?

Hi Mike,

I observed that several function objects (callbacks) are indirectly wrapped in unique pointers. Why?

e.g.
::std::unique_ptr<::std::function<void(display_surface& sfc)>> _Draw_fn;
::std::unique_ptr<::std::function<void(display_surface& sfc)>> _Size_change_fn;
::std::unique_ptr<::std::function<::std::experimental::io2d::rectangle(const display_surface&, bool&)>> _User_scaling_fn;

a std::function object can be empty and manages its memory allocation accordingly, because it implements type erasure. There is no need for an additional indirection. A callback that is not set still is checkable against nullptr. and checking the unique_ptr against nullptr is not enough, since the wrapped function object still could be empty.

Sorry for being terse, but I try to make it work on my mac (using xlib) and I am chasing why my input key events do not work.

Regards
Peter.

Consider removing most of the path functionality from the context and add it to the path type instead.

Right now context is a bit unwieldy (by design). One planned improvement is to make path objects first class citizens which are manipulated without going through the context. Then to draw a path you would simply set it on the context, set whatever other state needed setting, and invoke the appropriate drawing command (paint, fill, stroke).

Add support for CMake

Many projects build using CMake. Would adding a CMakeLists.txt file be welcome?

Consider moving most of the text functionality that is in context to the appropriate font objects

Similar to paths, there's a lot of text-related functionality that is part of context which should likely be moved out into one or more of the font-related types. A further reorganization of the font types might logically follow this.

Create CMake build files

Drop shared ownership semantics and use move-only semantics for those types instead

There are pros and cons to each. I plan to present this topic in Issaquah and make the case for shared ownership semantics. The only two types that have that now are shared_ptr and shared_mutex. This is ultimately a question for both LEWG and SG13. A veto by either would necessitate changing semantics, with the (to me) next best candidate being move-only semantics due to the expense of copying GPU resources that value semantics would incur.

Pick an endianness and stick with it.

Right now the endianness of image data is dependent on the architecture of the machine. We should consider resolving that endianness will always be little or big when presented to the user so that creating an image_surface from data or manipulating a returned vector of image_data will be portable without regard to architecture.

If we adopt the suggestion in issue #7 to create an image_surface::get_data_ptr function then for that case the developer would still need to pay attention to endianness since they'd be manipulating a raw data pointer, but that would be part of the price paid to get the perf increase there.

Eliminate "out" parameters where possible.

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

Whenever a function returns void and passes its actual return value as an "out" parameter (originally a pointer in C, which has been transformed to a non-const reference in C++), eliminate the out parameter and return the value directly. Disregard if multiple values are returned unless those values can be converted into some type (e.g.:
void foo(double& x, double& y);
should become
point foo();
if a point type is added).

Test and fix the Autotools build files

surface class missing member functions

Fixed already in the code. Needs to be fixed in the proposal's Technical Spec section.

Library rather than executable

I have locally converted the RefImpl to a static library and the Asteroids game to an executable. It's quite trivial, being only a setting in the vcxproj file; is that all we need? Or do we want to add dynamic linking with this change?

Rendering surface is arbitrary

nevermind.

Eliminate rectangle_list and use vector<rectangle> instead.

The only reason this hasn't happened already is that rectangle_list has a status and can potentially be defective. When we address error handling, this impediment should disappear, leaving the road to this change wide open.

Unexpected problem with functions getting a surface as argument per reference

Hi,

I wrote the following code to create a subsurface of the surface that "context" is drawing to:

auto subsurface = drawing::surface(context.get_target(), 10.5, 11.0, 50.0, 50.0);

However, this does not work, because get_target() returns a surface by value and surface() expects a reference.

Search for "surface&" in drawing.h suggests that there are lots of other functions which cannot be given the result of get_target() or get_group_target(), e.g. the binding for create_similar_image, the surface_pattern constructor and set_source_surface().

I think that this is (loosely) related to issue #20, because e.g. move-only semantics would make the binding for create_similar_image require a reference argument...

Since this behavior was unexpected to me, I wanted to report this. Feel free to decide that this (IMO) weirdness is the way that it should be and just close this.

Cheers,
Uli

Add support for loading from/saving to TIFFs

region should have a ctor that takes a rectangle_list

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

(Note: The title of this issue speaks for itself; there's nothing to add.)

Consider eliminating user data functionality and user_data_key class.

This functionality is unlikely to be used by anyone other than implementers. Since implementers are free to implement functionality if needed using the naming schemes defined in the standard, we can and should eliminate this functionality from the API.

Rename image_data_format to image_file_format and add image_file_format::tiff

Add class templates for a point type and a rectangle type

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

We should add in a point type to replace all of the "double x, double y" parameter pairs. We should also consider making it a class template and consider scrapping rectangle and rectangle_int in favor of a rectangle class template type.

Provide a few simple Win32 wrappers to simplify the windowing portions of the demo.

To eliminate the need for individual global variables, it would be nice to provide a few simple wrappers for the Win32 interactions. The main motivation for this is to minimize the 'noise' of the Win32 code, and provide more emphasis on the way the drawing code works.

Unreasonably slow

I'm developing another graphics library: Anti-Grain Evolution (https://github.com/tyoma/agge). It is based on Maxim Shemanarev's AGG, but is 3 times faster (and leaner in terms of functionality).
So today I performed a simple test and it turns out path drawing in io2d is 6 times slower than I have in AGGE. Should I switch to antialias(io2d::fast) it becomes 3 times slower. But agge always draws in maximum possible quality (it does analytic antialiasing).

This is how I draw path (I eliminated all the obvious memory allocations I could find in the measured interval, _surface and _spiral are created on resize, but never in drawing cycle):

     timings.clearing += stopwatch(counter);

     _surface->line_width(3);
     _surface->line_cap(io2d::line_cap::butt);
     _surface->line_join(io2d::line_join::miter);
     _surface->stroke(io2d::rgba_color(0.0, 154.0 / 255, 1.0));
     _surface->path(*_spiral);

     timings.rasterization += stopwatch(counter);

Can you tell if I'm doing something wrong or is it just Cairo that is so slow?
Being stateful (which is so not C++ish) io2d could make some use of caching, I think...

Below you can see the results are quite similar (all the pixels drawn are within the tolerance of 4 levels), but the timings are hugely different.

Investigate the usefulness, if any, of the device type

If it has no real use, eliminate it.

Check with SG6 re: a matrix type

Someone should check in with SG6 to see whether they are considering or have considered a generic matrix type. We have need of a matrix for affine transformations and so if it's something they are already considering or would be receptive to, then it makes sense to leave it to them (and try to corral someone to write a proposal if necessary) and for us to then use the result.

Project is not compiling with the defaults settings on linux

The default configuration uses c++11 with gcc (but c++14 with clang), making std::make_unique impossible to compile. Running grep -rl 'c++11' ./ | xargs sed -i 's/c++11/c++14/g' solves the problem.

Remove format::a1

General improvements to Win32 code

(n.b. Jason is already working on cleaning up code organization and turning the Win32 boilerplate into a class for cleaner, friendlier use; these changes will not intentionally duplicate any of that.)

Arcs do not render properly when the path has a matrix with rotation as its current matrix.

Bleh. I'm not going to close this issue until every path item has tests for every type of matrix along with non-default origin variants and passes them (visually). This should be unique to arc, but better to be safe.

Missing virtual dtors for base classes

Various base classes are missing virtual destructors in violation of rule 2.14.

Resizing Win32 window causes flickering of the rendered image.

Resizing appears to work properly in the rendered win32 window. However, it would be nice if we can eliminate the flickering when resizing the window.

Convert from sRGB when importing images then back to sRGB when exporting images

The compositing doesn't appear to be correct. See: cpp-io2d/io2dts#37 for information about how to test that the results are correct.

Graphics backend implementations need to be isolated

I've looked at the proposal and from what I can tell from both the proposal and this reference implementation, either the entire graphics subsystem has to be reimplemented for every graphics renderer or graphics backed implementations would be integrated into the STL/libstdc++/libc++.

Instead of being integrated or have entire subsystems reimplemented several times, the specification should isolate the rendering in some manner. I suggest making an interface class for graphics backends to implement. This would minimize the amount of duplicated code (and accidental bugs) while maximizing the number of possible graphics backend implementations (unlimited).

If this is already the case, please update the spec and reference implementation and tell me so I can stop stressing out about this.

Alter PNG-related transform rules for better clarity (i.e. static factories instead of ctors)

Suggested from cairo mailing list: http://lists.cairographics.org/archives/cairo/2014-January/024966.html .

Right now the image_surface ctors that result from cairo's PNG functionality disappear into the framework without any hint that that is what they are. This could be fixed by, e.g., creating static factory methods which perform the same construction but have names which make clear that these are from the PNG functionality.

Remove extend none

This extend mode, despite being the default in cairo, is rarely what people want (interpolating to a border of transparent black). The concept of extend none does not exist in most other graphics APIs (CoreGraphics, Direct2D, Direct3D, HTML Canvas) and so it feels like adding it to this standard adds undue implementation burden.

Change format from an enum to a struct for extensibiity

From Bengt Gustaffson: https://groups.google.com/a/isocpp.org/d/msg/std-proposals/IaaG316uPjo/p-DoZqnLMwQJ

The suggestion is that the format type should be changed from an enum class to a struct to get an open ended set of formats and to include the subpixel order info (e.g. exposing for RGBA32 whether R is the high order or low order byte, etc. for the position of G, B, and A, with related mechanisms for other format types) in the struct.

Consider having image_surface return a pointer from image_surface::get_data and take a pointer from set_data

Suggested on the cairo mailing list: http://lists.cairographics.org/archives/cairo/2014-January/024966.html

Currently calling get_data on image_surface creates a copy of the buffer data and returns it in a vector and set_data takes a vector. Both of these are performance sinks so it might make sense to just return the data pointer and set data from a pointer directly. The tradeoff is a loss of safety so it might be better to keep it as is since people shouldn't be calling these functions often anyway.

We could overload set_data to take a ptr, height, and format and create a get_data_ptr function though, thus giving the option for safety and the option for speed at the expense of safety. That's worth considering.

Does not run on Windows 7 due to CreateFile2 call.

One of the DLLs (zlib1.dll) is currently built in a way that makes it use CreateFile2 which is a Windows 8.0+ function. There's also a single use of CreateFile2 in entrypoint-win32.cpp that needs to be changed to CreateFileW.

Add Rocks In Space to the Autotools build files