fyne-io / fyne

Cross platform GUI toolkit in Go inspired by Material Design
https://fyne.io/
Other
24.53k stars 1.36k forks source link

draw rounded edges in Rectangle #1090

Closed MJacred closed 1 year ago

MJacred commented 4 years ago

Is your feature request related to a problem? Please describe:

For better readability and a more diverse collection of shapes. Different Components in Material Design have them, like Material buttons, tags/chips, cards, ...

Is it possible to construct a solution with the existing API?

desired outcome not possible at this time

Describe the solution you'd like to see:

define in canvas/Rectangle.go Rectangle struct additional members: cornerX1, cornerX2, cornerY1, cornerY2,
then define defaults in theme/theme.go per Widget: Makes sense for Button, Entry, Select, PopUp, TabContainer Button, ProgressBar, Spinner (wip). And other widgets which will probably come later: Frame, Switch.
Maybe let the user overwrite them in code?

see border radius in CSS

andydotxyz commented 4 years ago

I’m a little torn here on whether the individual corner configuration is needed? Adding “CornerRadius” to Rectangle could be a nice addition. However if it was to be 4 new fields then it might hint towards a RoundedRectangle type that offers more flexibility?

Just some thoughts really - no conclusion other than it sounds like a good idea in principle.

MJacred commented 4 years ago

Hm.. one CornerRadius probably suffices. I proposed the 4 for maximum configuration possibility.

I'd have no problem with an extra RoundedRectangle, which embeds Rectangle. So that both work harmoniously together, we'd probably need a Rect interface which both rectangle structs implement?

andydotxyz commented 4 years ago

The way that we add APIs here is based on a user requirement and exposing the least possible changes to support that - so maximum configuration isn't really a mantra we follow. If such configuration is needed we could add it as long as the API is kept clean and simple.

The canvas package uses only concrete types as they are used to pass through to a rendering engine so in that space interfaces don't usually work. With Go a Rectangle type could be embedded inside a RoundedRectangle - though whether that is cleaner than a small amount of duplication might be up for debate.

fpabl0 commented 3 years ago

@andydotxyz Isn't it possible to just add a new field to canvas.Rectangle called BorderRadius? Then the canvas.Rectangle could be any kind of rectangle rounded or not. BorderRadius could be private and defined from Theme API. I am not sure how this can be implemented but looking at internal/painter/draw.go the method DrawRectangle could use rasterx.AddRoundRect instead of rasterx.AddRect to enable border radius. This feature would be very helpful to make the current widgets more beautiful :)

jtlapp commented 2 years ago

Would the dev need to implement anti-aliasing for this, or does a suitable API already exist?

I'm still making a go/no-go decision about whether to implement my app in Go + Fyne. It's possible that I could add this feature, if it isn't done by then.

andydotxyz commented 2 years ago

To add new canvas primitives (or features to them) we need to add in the software and OpenGL renderers. The way that antialias works varies in this case. For the software renderer you can use any drawing API that can do curved corners and that will probably anti-alias. For the OpenGL it would mean replacing the simple rectangle draw with a shader (similar to the line implementation). You can see these things in action at internal/painter/*

renlite commented 2 years ago

In draw.go you are using module rasterx to draw Line, Rect and Circle. Would it work, to extend draw.go with func DrawRoundedRectangle(...), which calls the func DrawRectangle(...), if no corner radius (1,2,3,4) are set as parameter? The module rasterx has a func AddRoundRect(minX, minY, maxX, maxY, rx, ry, rot float64, gf GapFunc, p Adder) {...} which offers the functionality. (AddCircle() is already in use)

andydotxyz commented 2 years ago

Yes that would work, but it is a fallback for when OpenGL rendering can’t deal with the object. As having no border is currently drawn as hardware accelerated texture you might need the fallback mechanism to work differently than you described. Or add it to GL renderer as well :)

renlite commented 2 years ago

So far I understand the Circle object is always rendered as fallback and for OpenGL rendering of a RoundRect there would be a solution necessary like: Texture 9-slices (Triangle) or using Polygons. Is this correct?

func (p *glPainter) drawTextureWithDetails(o fyne.CanvasObject, creator func(canvasObject fyne.CanvasObject) Texture,
    pos fyne.Position, size, frame fyne.Size, fill canvas.ImageFill, alpha float32, pad float32) {

    texture := p.getTexture(o, creator)
    if texture == NoTexture {
        return
    }

    aspect := float32(0)
    if img, ok := o.(*canvas.Image); ok {
        aspect = painter.GetAspect(img)
        if aspect == 0 {
            aspect = 1 // fallback, should not occur - normally an image load error
        }
    }
    points := p.rectCoords(size, pos, frame, fill, aspect, pad)
    vbo := p.glCreateBuffer(points)

    p.glDrawTexture(texture, alpha)
    p.glFreeBuffer(vbo)
}

func (p *glPainter) drawCircle(circle *canvas.Circle, pos fyne.Position, frame fyne.Size) {
    p.drawTextureWithDetails(circle, p.newGlCircleTexture, pos, circle.Size(), frame, canvas.ImageFillStretch,
        1.0, painter.VectorPad(circle))
}

...

func (p *glPainter) drawRectangle(rect *canvas.Rectangle, pos fyne.Position, frame fyne.Size) {
    if (rect.FillColor == color.Transparent || rect.FillColor == nil) && (rect.StrokeColor == color.Transparent || rect.StrokeColor == nil || rect.StrokeWidth == 0) {
        return
    }
    p.drawTextureWithDetails(rect, p.newGlRectTexture, pos, rect.Size(), frame, canvas.ImageFillStretch,
        1.0, painter.VectorPad(rect))
}
andydotxyz commented 2 years ago

You are right that Circle still needs to have acceleration added. You can see the Line object which does have OpenGL code - which is more desirable of course. Rectangle is so heavily used I would worry about moving it to a slower renderer

renlite commented 2 years ago

During my attempts to draw a rectangle as primitive GLObject I realized that drawLine(...), drawRectangle(...) and so on are called twice on first and following render process with same param values. Is this necessary for layout or rendering?

I switched the original drawRectangle(...) to fragmentshader and vertexshader (GLSL) and the try seems to work. image Next step would be to try to implement a drawRoundRectangle(...) method with 9-slices.

I saw there is the lib raylib, which offers all the core gl functionality in a easier way and many base shapes (RoundRec), that are rendered as vertexshader. Raylib supports many platforms and OpenGL/ES versions. Wouldn't it be easier to use core modules of raylib for all backend OpenGL rendering?

andydotxyz commented 2 years ago

During my attempts to draw a rectangle as primitive GLObject I realized that drawLine(...), drawRectangle(...) and so on are called twice on first and following render process with same param values. Is this necessary for layout or rendering?

They should not be called twice with identical values as we have a cache to handle this. It is possible that the object values were the same but the canvas scale or texture scale were changed due to the window being realised on one screen then moved to another by the window manager (for example).

Wouldn't it be easier to use core modules of raylib for all backend OpenGL rendering?

I don't know the library but it looks like C code and our OpenGL work is using the Go bindings - I would prefer to keep it out of C as much as possible, unless there is a sensible reason like a large performance gain.

andydotxyz commented 2 years ago

I switched the original drawRectangle(...) to fragmentshader and vertexshader (GLSL) and the try seems to work.

Do remember to double check that you have not invoked with a borderless rectangle, that already uses OpenGL shaders (it is only when a border radius is set with non-transparent border that it would use the old software fallback).

renlite commented 2 years ago

I have changed only in https://github.com/fyne-io/fyne/blob/master/internal/painter/gl/draw.go the drawRectangle(...) to:

func (p *glPainter) drawRectangle(rect *canvas.Rectangle, pos fyne.Position, frame fyne.Size) {
    println("***Rectangle***")
    points := p.rectCoords(pos, rect.Size(), rect.StrokeWidth, frame)
    vbo := p.glCreateRectBuffer(points)
    p.glDrawRect()
    p.glFreeBuffer(vbo)
}

And I use canvas.NewRectangle() or widget.NewButton(). You are right only the Button ist called twice, sorry. image

raylib has many language bindings and for golang too. I don't know what the overhead would be, but maybe a fallback would not be necessary then.

Jacalz commented 2 years ago

raylib has many language bindings and for golang too. I don't know what the overhead would be, but maybe a fallback would not be necessary then.

That is not going to be a working solution in our case. Linking to C-projects unfortunately introduces another dependency that needs to be installed at both compile and runtime. It it does not quite work as regular Go code and because of our effort to be as statically linked as possible, we need to be very careful with new dependencies (or don’t add them at all).

andydotxyz commented 2 years ago

That is not going to be a working solution in our case. Linking to C-projects unfortunately introduces another dependency that needs to be installed at both compile and runtime.

Unless it is statically compiled in like with GLFW :)

Bluebugs commented 2 years ago

I would like to point out that there are cost and risk to consider by pulling non go dependency in. The big one, is that you now depend on another community to provide what you need. You also need to know that other language. Past experience, tell me that crossing language barrier means few people will be able to contribute and help address issues. Would really prefer a go dependencies here for that reason.

Jacalz commented 2 years ago

That is not going to be a working solution in our case. Linking to C-projects unfortunately introduces another dependency that needs to be installed at both compile and runtime.

Unless it is statically compiled in like with GLFW :)

That might make the binaries significantly bigger if the projects are very large. I don’t know if it even can strip out the unused parts of the C code. Probably not.

renlite commented 2 years ago

Last days i implemented an alpha version of a round renctangle as primitive GLObject in the fork: https://github.com/renlite/fyne It is a prototype and base work you could build on. Following files are changed in the fork-repository:

Some notes:

image

andydotxyz commented 2 years ago

This is super cool, thanks for working on this. I think we should hide the concept of segments though - the examples when you use it is more like a path than a rounded rectangle, and if not set appropriately it looks blocky. If required in the implementation I think we should automatically work it out internally.

Agreed that percentage isn't intuitive. If you have a rectangle height 10 with radius 5 it should join in the middle. Also I think it would be cleaner if the default constructor assumed all radii were the same, simply passing a radius float32 which will be used appropriately within the constructor. Last thought - can we antialias the edge?

renlite commented 2 years ago

I turned antialias on but I don't see much difference? Does it work? glfw.WindowHint(glfw.Samples, 4) gl.Enable(gl.MULTISAMPLE)

image

Bluebugs commented 2 years ago

Just a few comments here. I agree with @andydotxyz, having segment in the rounded rectangle primitive seems to be a work around from not having a path primitive on the canvas and make the API harder than it should be. Also please do not turn on MULTISAMPLE. It is making everything a lot more costly to run and will impact hardware that run on battery tremendously (reducing performance on other platform too). I do not think it is necessary to have it on to manage to get a smooth edge, but it would require to do the anti-aliasing manually (via the shaders).

andydotxyz commented 2 years ago

I turned antialias on but I don't see much difference? Does it work?

Yes, that looks much less jagged.

andydotxyz commented 2 years ago

I do not think it is necessary to have it on to manage to get a smooth edge, but it would require to do the anti-aliasing manually (via the shaders).

I guess there may be some inspiration available in the line renderer that was contributed which does anti-aliasing without the multisample enabled.

renlite commented 2 years ago

New version with antialiasing and some changes. https://github.com/renlite/fyne image

Bluebugs commented 2 years ago

@renlite I quickly read through your change on your tree and it seems you have addressed our past concern. Would you mind explaining a little bit how you are doing things as the commit are a little bit difficult to follow at the moment. I would expect you would rebase/squash/make independent meaningful commits before doing a PR, but before doing that additional work just doing a quick description of what you have done will be easier and quicker.

renlite commented 2 years ago

If the results are OK for you the next steps would be to comment the code and perhaps to paint a smaler inner shape to be able to define the stroke of the shape, which doesn't work at the moment.

andydotxyz commented 2 years ago

This is really amazing work thanks. Just one thought on your comments above - you say next steps include "next steps would be to comment the code " but you should know (more info in our Contributing docs) that we prefer readable code over comments - mantra "If you need a comment the code is not clear enough" (there are just a couple of notable exceptions and graphics may sometimes be just that).

renlite commented 2 years ago

@andydotxyz Yes, you're right. What I mean is a short description in front of the method.

@Bluebugs image image image image image

Alpha not 255.0: image blue_gray := color.NRGBA{R: 83, G: 140, B: 162, A: 180}

image blue_gray1 := color.NRGBA{R: 134, G: 174, B: 189, A: 255}

Bluebugs commented 2 years ago

Thanks @renlite, that looks pretty good actually. Regarding your point about only supporting opaque colours, I would believe it would make the shader slightly more complex to address it, but should be doable. Is it something you can look at or will you need help looking at it?

Bluebugs commented 2 years ago

I think I didn't really share my idea well for the shader, but maybe adding another parameter that describe which of the 4 quarter of a circle need to be drawn would work for this.

renlite commented 2 years ago

@Bluebugs Sorry for late reply I don't have much time at the moment to share my two ideas / directions to solve the transparent color problem. Could you please explain what you mean with extra parameter for the "4 quarter of a circle" for the fragment shader.

I think the problem is the overlapping of some line pixels with some circle segment pixels. That happens because I am using the original line implementation which paints the inner and outer stroke + feather of the line. Maybe a new line rendering painting only the outer side of a line could help to draw the line without overlapping the circle parts. I'm not sure if its possible to paint the line exactly along the edge of the circle without empty pixels at some positions.

image

In other case the fragment shader should know if a pixel is overlapping and in this case no color should be painted? Maybe there is a solution with GLSL?

renlite commented 2 years ago

@Bluebugs Help is welcome for FragmentShader of FlexRectangle. A solution for "Alpha < 255" in GLSL without the need to chnage the current go-code - as described above - would be cool.

    #version 110
    uniform vec4 color;
    uniform float lineWidth;
    uniform float feather;

    varying vec2 delta;

    void main() {
       if ( delta != vec2(0.0, 0.0) ){
            float distance = length(delta);

        if (distance <= lineWidth - feather) {
                gl_FragColor = color;
            } else {
                gl_FragColor = vec4(color.r, color.g, color.b, mix(color.a, 0.0, (distance - (lineWidth - feather)) / feather));
            }
        } else {
            gl_FragColor = color;
        }
    }
Bluebugs commented 2 years ago

It might not actually be possible. I was thinking of using discard in the shader to just not do anything which would mean there would not have been any conflict for that pixels, but I don't see how you could be precise enough to do so.

I think the solution is only if the outline can be drawn in the same shader that does the rectangle. This way only one pixel is every pushed to the screen and the blending would work. Not to sure how that would fit in your work.

Bluebugs commented 2 years ago

I actually did get an idea last night that would work and shouldn't be to hard to implement. It does require shader change still. My idea would be to render the outline in a 8bits buffer and use it as a mask for the rounded rectangle shader. The rounded rectangle shader would receive both the color of the outline and the fill color too. This way when deciding to color a point, it can check if the mask color is set, in which case it would be a function of mask outline color + (1 - mask) gl_FragColor.

renlite commented 2 years ago

Sorry, but I don't understand your idea "to render the outline in a 8bits buffer and use it as a mask for the rounded rectangle shader". Could you please explain it further or give me an example what you mean.

Yesterday I started to implement a new version of the line rendering with stroke and feather only on one side. That should make it possible to draw the base line (of the line) exactly on the outline of the circle segement and then to use stroke + feather for antialiasing. In that case I would change the FragmentShader to transparent color (Alpha 0,0) and if necessary adapt the if statement for (distance <= ...). Do you think it could work?

if (distance <= lineWidth - feather) {
>>>     gl_FragColor =  vec4(0.0, 0.0, 0.0, 0,0)    <<<
    } else {
        gl_FragColor = vec4(color.r, color.g, color.b, mix(color.a, 0.0, (distance - (lineWidth - feather)) / feather));
    }

But I would like to understand your solution.

Bluebugs commented 2 years ago

I was thinking of creating a FBO and use a texture with only one component, like GL_RED_INTEGER, as a render target. Using that FBO to render the outline of the button in that texture and then the resulting texture can be used in the rounded rectangle shader to render the outline and the inside content without "double" rendering and avoid the alpha problem when rendering the pixel twice.

For reference a good introduction on FBO: https://learnopengl.com/Advanced-OpenGL/Framebuffers

Your idea might work, I am not certain about rounding between two execution, but likely it should work too. At least at high level, I think it is likely to work.

renlite commented 2 years ago

After some thinking I only changed the original method flexLineCoords(), to paint only the outside of the line (FramentShader is not changed):

    return []float32{
        // coord x, y normal x, y
        x1, y1, normalX, normalY,
        x2, y2, normalX, normalY,
        x2, y2, 0.0, 0.0,         <<<
        x2, y2, 0.0, 0.0,         <<<
        x1, y1, normalX, normalY,
        x1, y1, 0.0, 0.0,         <<<
    }, halfWidth, featherWidth

As far as I can see the transparent colors and anitaliasing seem to work too.

image

https://github.com/renlite/fyne Would you please check it with other resolutions and other variations?

image image image image

andydotxyz commented 2 years ago

Nice! Great work :)

renlite commented 2 years ago

https://github.com/renlite/fyne @andydotxyz @Bluebugs Before I make a pull request I would like to share some information about the new version: image

Would you please inform me if the branch is ok for a pull request.

andydotxyz commented 2 years ago

This looks really great thanks - very promising indeed. I'm not sure about the options pattern though, we don't use it any where else, preferring constructor functions and field manipulation - smaller APIs that are easier to discover IMHO. (plus we can't make breaking changes to the existing NewRectangle function.

To feed back on the CreateRenderer issue can you please point to the line of code that's not working?

andydotxyz commented 2 years ago

I tried to compile but get this:

❯ go run .
panic: failed to compile 
    #version 110
    attribute vec2 vert;
    attribute vec2 normal;
    attribute float switch;
    attribute float lineWidth;
    attribute float feather;

    varying vec2 delta;
    varying float switch_var;
    varying float lineWidth_var;
    varying float feather_var;

    void main() {
        switch_var = switch;
        lineWidth_var = lineWidth;
        feather_var = feather;
        if ( normal == vec2(0.0, 0.0) ) {
            gl_Position = vec4(vert, 0, 1);
        } else {
            delta = normal * lineWidth_var;
            gl_Position = vec4(vert + delta, 0, 1);
        }
    }
: ERROR: 0:5: 'switch' : Reserved word. 
ERROR: 0:5: 'switch' : syntax error: syntax error

goroutine 21 [running, locked to thread]:
fyne.io/fyne/v2/internal/painter/gl.(*glPainter).Init(0xc000702040)
    /tmp/fyne/internal/painter/gl/gl_core.go:303 +0x35b
fyne.io/fyne/v2/internal/driver/glfw.(*window).create.func2()
    /tmp/fyne/internal/driver/glfw/window.go:1469 +0x6b
fyne.io/fyne/v2/internal/driver/glfw.(*window).RunWithContext(0xc000126600, 0xc000012010)
    /tmp/fyne/internal/driver/glfw/window.go:1354 +0x4f
fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).startDrawThread.func1()
    /tmp/fyne/internal/driver/glfw/loop.go:232 +0x14c
created by fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).startDrawThread
    /tmp/fyne/internal/driver/glfw/loop.go:224 +0xef
exit status 2
renlite commented 2 years ago

Thanks for your feedback. Sorry, I don't know why the GLSL compiler doesn't work on your machine. I changed the attribute switch to colorSwitch. Now it should work.

    vertexRectShaderSource = `
    #version 110
    attribute vec2 vert;
    attribute vec2 normal;
    attribute float colorSwitch;
    attribute float lineWidth;
    attribute float feather;

The reason I have selected the options pattern is not to break the original NewRectangle API and to offer further options in simple form. The new version could be used as before and can be extended with new and defined options. If I change the param to type Rectangele struct, I will break the existing code. My first implementation was:

func NewRectangle(color color.Color) *Rectangle {}
// #1
func NewRectangleWithRadius(color color.Color, radius fyne.RectangleRadius) *Rectangle {}
func NewRectangleWithStroke(color color.Color, ...) *Rectangle {}
func NewRectangleWithRadiusAndStroke(color color.Color, ...) *Rectangle {}
// #2
func NewFlexRectangle(rect Rectangle) *Rectangle {}
// #3 options patter (WithStroke and WithRadius are optional)
NewRectangle(yellow, WithStroke(orange, 5.0), WithRadius(rectRad9))

Should you prefer an other implementation, please give me the names and a short API description.

Button not working:

func (b *Button) CreateRenderer() fyne.WidgetRenderer {
    b.ExtendBaseWidget(b)
    seg := &TextSegment{Text: b.Text, Style: RichTextStyleStrong}
    seg.Style.Alignment = fyne.TextAlignCenter
    text := NewRichText(seg)
    text.inset = fyne.NewSize(theme.Padding()*2, theme.Padding()*2)

    >>>    background := canvas.NewRectangle(color.NRGBA{R: 255, G: 200, B: 0, A: 180})      <<<
        ...
        r := &buttonRenderer{
                ShadowingRenderer: widget.NewShadowingRenderer(objects, shadowLevel),
                background:        background,
                tapBG:             tapBG,
                button:            b,
                label:             text,
                layout:            layout.NewHBoxLayout(),
            }

To see if a button works I changed the canvas.NewRectangle(color.NRGBA{R: 255, G: 200, B: 0, A: 180}) to a fix color.

andydotxyz commented 2 years ago

The new version could be used as before and can be extended with new and defined options. If I change the param to type Rectangele struct, I will break the existing code.

I don't quite understand this sorry. Why was using Rectangle fields like we have for the other features not working?

I changed the attribute switch to colorSwitch. Now it should work.

This is now working thanks, though when I resize the window I get some artefacts (macOS) - have you seen this too? Screenshot 2022-05-02 at 14 27 16

To see if a button works I changed the canvas.NewRectangle(color.NRGBA{R: 255, G: 200, B: 0, A: 180}) to a fix color.

It may be because the Button code sets background colour to match theme, which is transparent by default.

renlite commented 2 years ago

As you can see on Win10 it works without some artefacts after resize of the window. It is difficult to understand because the below rectangles (yellow/orange)

    // >> composition
        rr6,
        rr7,
        txt1,
        // >>
        rr9,

are painted correctly and it happens mostly on the right bottom. This is slice 9 which ist the last coords calculation. Could you please test it only with two objects or check it on linux or mobile?

image

I noticed that the method func (p *glPainter) glDrawRect(fillCol color.Color, strokeCol color.Color, trianglePoints int32) is called twice for every object. The call stack starts in painter.go method func (p *glPainter) Paint(obj fyne.CanvasObject, pos fyne.Position, frame fyne.Size).

It seems that in a container.NewWithoutLayout the button does not work. With BoxLayout the border of the buttons and the animation is visile. image

My idea was if I change the original method from func NewRectangle(color color.Color) *Rectangle to func NewRectangle(r *Rectangle) *Rectangle it will break for example the existing code in

func (b *Button) CreateRenderer() fyne.WidgetRenderer{
...
    background := canvas.NewRectangle(theme.ButtonColor())
    tapBG := canvas.NewRectangle(color.Transparent)
...
}

A second or third parameter would break the existing code too, because canvas.NewRectangle is called only with one (color) parameter. Not to break the existing code I thought to implement one init method like func NewRectangle2(r *Rectangle) *Rectangle or more specific methods like func NewRectangleWithStroke(fillColor color.Color, strokeColor color.Color, strokeWidth float32) *Rectangle and func NewRectangleWithRadius(fillColor color.Color, radius fyne.RectangleRadius) *Rectangle and func NewRectangleWithRadiusAndStroke(,,,) *Rectangle for all variations.

func NewRectangle(color color.Color, options ...RectangleOption) *Rectangle does not break existing code and offers static typed optional parameters with code completion. Only defined func can be used and further options could be added later.

Maybe func NewRectangle2(r *Rectangle) *Rectangle is the better option?

andydotxyz commented 2 years ago

As you can see on Win10 it works without some artefacts after resize of the window.

Perhaps it can be debugged during a PR?

It seems that in a container.NewWithoutLayout the button does not work.

Did you remember to add the appropriate Move and Resize calls? without them an item is not layed out and so tap targets likely 0x0.

Maybe func NewRectangle2(r *Rectangle) *Rectangle is the better option?

I don't understand why you would have the same input and output type for a constructor function...?

func NewRectangle(color color.Color, options ...RectangleOption) *Rectangle does not break existing code and offers static typed optional parameters with code completion.

Technically yes, but the last parameter would only be "Since: 2.3" but these annotations are per-function (or type). Overall I don't see why we need a new pattern for constructing, the current situation with structs and fields is used everywhere else and is also static typed with code completion.

andydotxyz commented 1 year ago

This is now fixed on developed - @renlite is a real star :)