Add rounded edges to Rectangle

[renlite]

Description:

#1090
Implementation of a (Round)Rectangle as primitive Open GLObject.

• The property "fyne.RectangleRadius" extends canvas.Rectangle struct.
• For the left and right side a different radius can be defined.
• For the left and right side different segments can be defined. Default is 8 segments for one rounded edge of a rectange. The higher the value the more triangles are calculated and rendered.
• A rectangle with stroke is rendered in one communication/stream to the GPU. A shape with opaque colors and stroke can be rendered with composition.
• If radius is 0 a normal rectangle will be drawn.
• Only required slices are rendered, so I hope the performance will be ok.

Limitation: Shapes with stroke and only one segment don't work properly. As a work around for shapes with opaque colors you could use composition. As this is not the main use for this canvas object I don't plan to implement it.

Checklist:

• Tests included.
• Formatter run with no errors.
• Lint run with no errors.
• Tests all pass.

Where applicable:

• GL ES and software-fallback not implemented
• Please check: rendering (method: drawRectangle https://github.com/renlite/fyne/blob/c24f9a641cf0bf39f2e0c0da1694aed85132c239/internal/painter/gl/draw.go#L110) is called twice
• Old method: drawRectangle was commented out
• See last discussion draw rounded edges in Rectangle #1090
• Example included. (https://github.com/renlite/fyne/blob/develop/example/flexrect.go)

[Jacalz]

Thanks for opening the PR. Would you mind changing the title name to be more specific about what the PR is doing (instead of which issue it fixes)? :)

[andydotxyz]

Amazing thanks!

Can you please put example code into the fyne_demo cmd rather than creating a new package for this PR?

[andydotxyz]

[x] Tests included. (https://github.com/renlite/fyne/blob/develop/example/flexrect.go)

When we say "tests included" we mean unit tests, not example code.
Please don't remove the other checks from the PR template - the "where applicable" can be removed, but the rest is required.

[renlite]

Amazing thanks!

Can you please put example code into the fyne_demo cmd rather than creating a new package for this PR?

Ok, I found what you mean. Expanded cmd/fyne_demo/tutorials/canvas.go.

[renlite]

Which lint tool should be used if any?

[Jacalz]

Which lint tool should be used if any?

The ones used in the static analysis action should always be used.

[Bluebugs]

This is pretty cool. Looking at the code, I realized that maybe it will lead to a faster frame rate when using the web backend. Sadly, something is broken there with this patch. I think it is a bug in the gl-js layer and not in this patch. I am going to look more into it, but please do not land this patch until confirmed that it work for the web backend properly.

[Bluebugs]

I figured out that there is no GL ES variant of the shader which lead all other backend to fail. I kind of half fixed it locally, but then I see errors in the web console:

WebGL warning: drawArraysInstanced: Vertex fetch requires 10, but attribs only supply 6.
WebGL warning: drawArraysInstanced: Vertex attrib array 2 is enabled but has no buffer bound.

[renlite]

I figured out that there is no GL ES variant of the shader which lead all other backend to fail. I kind of half fixed it locally, but then I see errors in the web console:

WebGL warning: drawArraysInstanced: Vertex fetch requires 10, but attribs only supply 6.
WebGL warning: drawArraysInstanced: Vertex attrib array 2 is enabled but has no buffer bound.

I havn't looked at GL ES so far. Shaders for GL ES are not defined and built at the moment nor implemented in eg. gl_es.go->Init(). My next steps are to split the method flexRectCoords() in draw.go to fullfill the lint checks. I hope the GL 110 and GL ES are compatible. I think in that case only the calling stack for GL ES must be changed.
Is GL ES used for mobile and in the browser?

[Bluebugs]

Yes, the GL and GL ES shader between 110 and 100 are very close.

[Bluebugs]

It is a very simple shader, there won't be a problem with OpenGL100 and GL ES 110. The shape is just a simple rectangle, no complexity there. Indeed the way it is currently written, it is the same radius for all corner. The benefit of that shader is that you also get linear gradient for the stroke and fill.

I kind of think your technique is closer to my other proposal to have a bunch of very simple independent triangular shape and build complex shape from them. I do think both work, but having a dedicated shader just for rounded rectangle would pay off considering the amount of time rounded rectangle shape are used in modern UI.

[renlite]

There is a first version of GPU rendered stroke rectangle. This prototype doesn't have round edges. The normalization of vertex shader is calculated on the GPU too.
https://github.com/renlite/fyne
Would you please try it.

[Bluebugs]

Just tested, it does work (as you say no round edges). This is on the github.com/renlite/fyne@develop branch for whoever want to test next.

[renlite]

@Bluebugs Thank you for the test.
New version is available on github.com/renlite/fyne@develop branch. Now with round rectangle GPU rendered. Example .\cmd\rect_100+\main.go uses round rects.

If it works too further and more complex tests will be prepared before pull request.

[Bluebugs]

Nice, it does work for me with the rounded rectangle. Maybe something to pay attention to when doing the PR, but the commit log looks weird.

[ErrorNoInternet]

There is a first version of GPU rendered stroke rectangle. This prototype doesn't have round edges. The normalization of vertex shader is calculated on the GPU too. https://github.com/renlite/fyne Would you please try it.

Looks like this for me, regardless of what card I'm using (notice the buttons)



(Those Xlib errors seem to appear when I press the PrintScreen button to take screenshots)

[renlite]

@ErrorNoInternet Thanks for the response! I assume flickering is gone.
This is an unexpected issue. It seems the coords are not OK. There is a new example .\cmd\rect_test\main.go. I changed the outside color to see the space in use and implemented a shell output to see the coords (not normalized) for the rect submitted to the shaders.

[ErrorNoInternet]

@renlite

Yup, flickering is gone.
However, I'm seeing different results with different cards again.

Intel:

NVIDIA:

[renlite]

I thought it has something to do with scale and went through the code. I didn't find the reason in the fragment shader.
The first result (@ErrorNoInternet) shows correct rectangle space from vertex shader, but the second one has less vertical space for the first two rectangles. All rectangles have not the correct centerPosition. Scale seems not to produce the error, the results between scale 1 and scale 1.2 are almost identical.
At the moment I don't know why the issue happens. To see the frame size I implemented a println and in the fragment shader, if the size is not x800.0 and y600 a red outer color will appear in the 'rect_test' example.

[renlite]

It seems the real size of the screen/window in the fragment shader is not the submitted frame size but a scaled form. Maybe I have to consider the scale in the fragment shader.

[Bluebugs]

I haven't looked at your code, but you might need to get the exact pixel size and not the windows pseudo pixels size which is scaled. I have found that a bit confusing as it is hard to see when it is scaled and when it is not. In your case, as things are sent to GL, you want to have the real pixels size.

[Bluebugs]

I can confirm it is a scale issue, you can try with different FYNE_SCALE value different from 1 and you will see the issue.

[andydotxyz]

I have found that a bit confusing as it is hard to see when it is scaled and when it is not.

Scaled values (our main coordinate system) are float32. Pixel coordinates (limited to the render/driver code) are int values.

Potential confusion arises later because OpenGL is working with floats from 0.0 and 1.0. However that is specific to the GL painter and not something that should impact other code.

[renlite]

Potential confusion arises later because OpenGL is working with floats from 0.0 and 1.0. However that is specific to the GL painter and not something that should impact other code.

Vertex coordinates are working different from fragment coordinates. Fragment coords can be normalized to 0.0 -> 1.0 if the real size of the screen is known, but this is not necessary. From this view some var names in the fragment shader are not correct, and should be changed later.

[renlite]

The code has been updated: pixScale is submitted to OpenGL and used in fragment shader. It has no impact on my hardware with scale 1.

[Bluebugs]

This does work for me and look pretty good. It is definitively reducing massively the amount of texture uploaded to the GPU.

I think that with the same shader, you should be able to provide the implementation for circle to be fully running on the GPU too without texture upload.

Just a quick review of the shader.

Division is slow and costly and 2.0/frame_size.x along with 2.0/frame_size.y are not changing for the entire run of the shader. I guess you can just precalculate them and just do vec4(vert.xscale.x - 1.0, 1.0 - vert.yscale.y, 0, 1) where scale.x == 2.0/frame_size.x and scale.y == 2.0/frame_size.y.

A shader get its entire code, including all the if branch, executed for every run of the shader. The if ( radius == 0.0 ){ in the shader will actually lead to a more costly run and the code for rounded rectangle should work just fine with radius == 0.0.

For people that are interested in reviewing the PR and might now know so much about the details of shaders, the way to think a bit about shader, they are executing things for multiple pixels, geometry, ... in parallel. That parallel execution is done using mainly a Single Instruction Multiple Data design. To make it work with loop and if construct, each instruction use a mask to disable/enable which pixels/geometry/... the instruction is manipulating. That's why if do not provide an optimization or shortcut as they do on the CPU, because all the branch are executed, but with a different mask.

Overall this feel very close to a PR and very early in our release cycle, would be amazing to get!

[renlite]

Does it work for @ErrorNoInternet and @andydotxyz too?

[Jacalz]

It seems to work for me on Intel UHD Graphics 620 but the grey rectangles go red when I resize the window (in the example application).

[renlite]

It seems to work for me on Intel UHD Graphics 620 but the grey rectangles go red when I resize the window (in the example application).

Thanks, the grey to red change is ok, it is the only possible type of debugging on the GPU. This will be deleted later.

[ErrorNoInternet]

It looks fine on my first monitor thats 1650x1080, however, when I drag it to my 1920x1080 monitor it looks like this:

The window also becomes larger, so I guess it just scaled up

[renlite]

Thank you for your feedback. That seems to be also ok. The last rect without round edges is not correct but this is because I did not scale the size in the shader for this rect type. There are open questions to discuss (as mentioned by @Bluebugs) whether to intergrate the simple rect or to write a separate shader. The simple rect (with stroke) is massively used so a simpler shader would save power.

The window also becomes larger, so I guess it just scaled up

I would say this behavior is ok too but I can't anwer this question.

[Bluebugs]

The simple rect (with stroke) is massively used so a simpler shader would save power.

This is absolutely true.

The window also becomes larger, so I guess it just scaled up

I would say this behavior is ok too but I can't anwer this question.

Yes, when moving window between screen with different scale, the window min size will be recalculated and most of the time it will be different due to a difference in pixels density that lead to a bigger window.

[renlite]

@Bluebugs @andydotxyz
I will clean up the code and prepare it for a PR. I think it is clearer to make a new PR with a link to this one. Do you agree?

@Bluebugs

Division is slow and costly ...

Yes, this is a good input to reduce calculations in the shaders for variables that are constant through all threads. I know that the fragment shader creates for all pixels of the rendering area a thread to paint the color, but is this true for the vertex shaders too ? I don't know if the vertex shader uses only the number (threads) of used points - in our case 6 = two triangles - or the whole number of pixels. If vertex shaders creates threads for all pixels too then it would be better to make the normalization in Go and not in OpenGL.
gl_Position = vec4(2.0*vert.x/frame_size.x - 1.0, 1.0 - 2.0*vert.y/frame_size.y, 0, 1);
attribute vec2 normal; are not used at the moment and the submit would happen in one buffer.

I think that with the same shader, you should be able to provide the implementation for circle to be fully running on the GPU too without texture upload.

Yes, this was my plan to do it with a new PR. Maybe with an own fragment shader. I think there is a faster version for circle only.

@Bluebugs @andydotxyz @Jacalz @ErrorNoInternet
To get optimized rendering I would create a separate CanvasObject for the Rectangle and RoundRectangle both inheriting from baseObject.
This idea has following reasons:

1. Better performance for Rectangle (massively used)
2. The fragment shader is prepared to support different strokes (top, bottom, left, right). RoundRectangle can have only one stroke width.
3. Because it is not necessary, the Rectangle is not antialiazed. I did not try it but I think it should work to create a Rectangle with the object RoundRectangle (Radius 0.0). This would be much more expensive (for many objects) but would offer smooth steps if desired by the programmer.

@andydotxyz could you please test the example /cmd/rect_test/main.go (branche develop) whether flickering is gone on your hardware?

[Bluebugs]

@Bluebugs @andydotxyz I will clean up the code and prepare it for a PR. I think it is clearer to make a new PR with a link to this one. Do you agree?

Absolutely, will be easier to do a proper review from a clean start.

@Bluebugs

Division is slow and costly ...

Yes, this is a good input to reduce calculations in the shaders for variables that are constant through all threads. I know that the fragment shader creates for all pixels of the rendering area a thread to paint the color, but is this true for the vertex shaders too ? I don't know if the vertex shader uses only the number (threads) of used points - in our case 6 = two triangles - or the whole number of pixels. If vertex shaders creates threads for all pixels too then it would be better to make the normalization in Go and not in OpenGL. gl_Position = vec4(2.0*vert.x/frame_size.x - 1.0, 1.0 - 2.0*vert.y/frame_size.y, 0, 1); attribute vec2 normal; are not used at the moment and the submit would happen in one buffer.

For the vertex shader, it should be per vertex. So having a bit of optimization there is still a good thing, but no need to do head scratching one :-)

I think that with the same shader, you should be able to provide the implementation for circle to be fully running on the GPU too without texture upload.

Yes, this was my plan to do it with a new PR. Maybe with an own fragment shader. I think there is a faster version for circle only.

I agree, there is a faster one for circle and it could enable adding more option later to it (like partial arc and stuff). Still, this is already a pretty good step forward, so I would be perfectly ok with just reusing the rounded rectangle shader for now.

@Bluebugs @andydotxyz @Jacalz @ErrorNoInternet To get optimized rendering I would create a separate CanvasObject for the Rectangle and RoundRectangle both inheriting from baseObject. This idea has following reasons:

1. Better performance for Rectangle (massively used)

2. The fragment shader is prepared to support different strokes (top, bottom, left, right). `RoundRectangle` can have only one stroke width.

I think having multiple stroke width for each side of the rectangle to be maybe a bit unnecessary at this stage. All the modern design I have seen are using the same stroke around the rectangle. It might be a marginal use case that would be better left to a generic vector drawing primitive.

3. Because it is not necessary, the `Rectangle` is not antialiazed. I did not try it but I think it should work to create a `Rectangle` with the object `RoundRectangle (Radius 0.0)`. This would be much more expensive (for many objects) but would offer smooth steps if desired by the programmer.

I don't think you need a new primitive, you just need to check in the painter if the radius is != 0 and switch the shader accordingly. I prefer to have less primitive exposed, but with more feature and put all the optimization logic in the painter as much as we can.

[andydotxyz]

When run on macOS I see this:

@Bluebugs @andydotxyz
I will clean up the code and prepare it for a PR. I think it is clearer to make a new PR with a link to this one. Do you agree?

Yes please, a nice clean start with the new code.

It may be worth noting that if Radius == Height/2 then the ends of the rectangle should be a semi-circle, but I don't think it is, so when adjusting the scaling calculations maybe check this too?

To get optimized rendering I would create a separate CanvasObject for the Rectangle and RoundRectangle both inheriting from baseObject.

I don't think the API should be designed around implementation details. We can internally load two different shaders based on whether Radius == 0 and/or StrokeWidth and StrokeColor settings as well. This way the user of the API does not have to worry and can turn radius on and off for the shape.

[renlite]

Some notes to the actual state:

• Code redesign with a lot of optimizations in vertex and fragment shaders.
• Implementation of scaling for all relevant params.
• Original cmd/fyne_demo/main.go should now work as expected.
• Split between Rectangle and RoundRectangle depending on Radius.
• In the example cmd/rect_test/maint.go is a try to paint a circle with round_rectangle.frag shader. I think it looks quite good.
• At the bottom of the example cmd/rect_test/main.go are two rectangles. The left one is rendered by rectangle.frag and the right one by round_rectangle.frag. The right rectangle is antialized and has a radius of 0.01.

Not implemented at the moment:

• To define radius by pixel and in percent. At the moment only pixel. I would say this needs a struct of Radius with both values. If both values are defined then use pixel or throw an exception.
• The circle shape is still a texture. Replace texture rendering with round_rectangle.frag shader if the result looks good.
• Shaders for OpenGL ES not started yet.

Please let me know if everything works as expected.

[ErrorNoInternet]

@renlite Works perfectly fine on both my monitors!

[andydotxyz]

Oh, for the screenshots addition it looks glitch-free on my HiDPI Linux laptop too:

Andy fyne_demo seems to be working perfectly, thanks!

[renlite]

Thank you for the tests. I will include the OpenGL ES path and the radius in percentage and then make a PR.