fabricjs / fabric.js

Javascript Canvas Library, SVG-to-Canvas (& canvas-to-SVG) Parser
http://fabricjs.com
Other
28.9k stars 3.5k forks source link

[Feature]: Updating width and height of an ellipse should update rx and ry and vice versa #10179

Open houssameddine-h opened 2 weeks ago

houssameddine-h commented 2 weeks ago

CheckList

Description

Hi all,

The way ellipses are implemented for now is: if you update rx or ry the width and height get updated accordingly, but not the other way around (if you update the width or height the rx and ry don't get updated) which is more intuitive to actually do in my opinion. The decision for this implementation was made here: #1062

This is a real problem, because when you try to resize an ellipse using the UI controls, it actually changes the width and height of the ellipse, which has no effect on the shape itself, but only updates the "container" of the shape.

It's easy enough to fix this in my own project, but I feel like everybody will have to do the same, since no one wants to resize a shape using the controls and having only the container changing.

The same behavior is implemented in circles, so we might fix that as well.

This is what you get when you try to resize an ellipse using the UI controls:

Screenshot 2024-09-25 at 23-33-37 React App

Current State

As a workaround for now, we can update these properties accordingly by listening to the relevant events.

Additional Context

No response

asturur commented 2 weeks ago

You can also just make some controls that update rx and ry instead of width and height. The resize controls, the one that changes the width, was designed only for the textbox. In general width/height drive changes in rect and textbox but are just a consequences of other properties in all other shapes.

houssameddine-h commented 2 weeks ago

I see your point @asturur, but if those controls were designed specifically for textbox, then why not designing specific control behavior for other shapes as well? I'm not sure of the amount of work that would take, but in the case of ellipses and circles it's not a lot.

I know that we should design the library to be generic, but since we already implemented different shapes, we should tweak the default controls to be specific to each shape.

houssameddine-h commented 2 weeks ago

Also I'm not sure how to change the default behavior of the controls without having to redefine them all.

My initial approach was to listen for the object:scaling event and update the object properties.

zhe-he commented 6 days ago

I also have this confusion. I think the code should be as simple as possible. I hope the ellipse only has width and height, and that rx and ry are read-only. Wouldn't the following be more convenient?

class Ellipse2 extends FabricObject {

    get rx() {
        return this.width / 2;
    }
    get ry() {
        return this.height / 2;
    }

    // If backward compatibility is not a concern, the two methods below can be removed. 
    // If users want to set rx and ry, let them set the width and height instead.
    // set rx(n: number) {
    //    this.width = n * 2;
    // }
    // set ry(n: number) {
    //    this.height = n * 2;
    // }
}
asturur commented 16 hours ago

i m against the 'viceversa' point. There is a large confusion in fabricJS about width and height.

width and height as properties you can change works good for images and rect and nothing else. On triangle works good only for triangles that have a flat base.

On text it make sense only to change width because height is a consequence

on polygon, paths and circle and ellipse are consequences of other properties r, rx, ry, points and commands.

In my opinion because of polygons and paths and text we have to move away from the width and height concept for most shapes. They need to exist internally as private properties and developers have to interact with the rx, r, ry, points and commands when trying to change a shape, while rect and image can continue to support width and height getters.

This is a major change tho

For what regards custom controls for ellipse and circle i can make a demo, it would help many people understand how to work with those. I ll do the one for ellipse that maybe is the most clarifying one