Poly custom control

[luizzappa]

Description

Custom poly controls as part of fabric.js core.

Fixes:

• Poly with skew
• Poly with flip
• Poly with strokeUniform

Closes:
#8359

Gist

  const poly = canvas.getActiveObject();
  poly.controls = fabric.controlsUtils.PolyControl.createPolyControls(
    poly.points.length, 
    { cursorStyle: 'crosshair' } //pass custom options
  );
  canvas.requestRenderAll();

In Action

https://codesandbox.io/s/custom-control-poly-95y2im

https://codesandbox.io/s/custom-control-poly-fix-flip-95y2im

[luizzappa]

@ShaMan123 , I was in doubt of how to expose the PolyControl class. I put it inside fabric.controlsUtils

[asturur]

What is the reason to have them as a class rather than a bunch of functions to construct a normal control with? Do they need to be different? if yes, why?

[luizzappa]

@asturur, I used class because I needed a new property (pointIndex) on the control to be able to relate a control to the polygon point (so that we can change the position of the point according to the mouse position).

But I think if I used factories and closure for that it would work too and we could just go with functions instead of class (I don't know if it has an impact on performance):

function factoryPolyPositionHandler(pointIndex) {
  return function polyPositionHandler(dim, finalMatrix, fabricObject) {
           //etc
  }  
}

function factoryPolyActionHandler(pointIndex) {
  return function polyActionHandler(eventData, transform,  x, y) {
           //etc
  }  
}

If it makes more sense, I can refactor.

[ShaMan123]

nice and clean.
I would rename the file to PolyControl.ts. We will eventually rename all I presume.
Use this syntax for method comments:

/**
 * comment
 */

And we should add tests.
I would expose this as part of src/controls/default_controls.ts as same as the rest in that file.

We discussed making controls as classes...
I will voice a few more points.
Since poly controls are very defined I don't see why not.
It is not like others that you mix and match.
I prefer creating a base class with all the wrappers and having the class lifecycle invoke them (since there is a generic lifecycle to all controls).
Then it is a question of subclassing specific parts instead of constructing and nesting wrappers.
Everything is moving to classes, why not controls as well?

[ShaMan123]

flip needs to be fixed

[luizzappa]

flip needs to be fixed

f1ab52c

https://codesandbox.io/s/custom-control-poly-fix-flip-95y2im

[luizzappa]

And we should add tests.

I'll work on it

I would expose this as part of src/controls/default_controls.ts as same as the rest in that file.

it's not clear to me how to expose inside default_controls.ts.. Do we create an object in the fabric namespace?

[luizzappa]

Are you guys updating fabric.js demos ( http://fabricjs.com/custom-controls-polygon )? Although it is now quite simple, perhaps it is worth it in terms of visibility.

[ShaMan123]

The website is considered deprecated and even dead.
We are going to rewrite it from scratch after v6 stabilizes.
Welcome to join to any effort you'd like.

[luizzappa]

Is better to have simpler code rather trying to handle edge cases that could be neutralized in a different way. Polygon editing is not a fabricJS feature so we don't have to support all the use cases of the polygons. Otherwise we have to get into things like:

• gradient fill, how does change the offsets of the colors when i change the points
• pattern fill, same as above
• editing of polygons inside containers with random transforms applied

In my opinion, if this is the driver, it is better to leave it as an example of use than in the core of the library. At least in the example there is a more detailed guide that allows the developer to understand and adapt to the needs of his application.

I agree that the transform data should carry every bit that can be useful to have context during the handler, i m not sure we can capture the index there, but again if you create a control and pass in the constructor the pointIndex you will have it available.

Typescript complains that there is no pointIndex property in the Control interface.

I ended up passing pointIndex inside transform.

Regarding editing a polygon under group that must be fixed.

It was supposed to work within a group as well, since calcTransformMatrix() calculates the final matrix of all planes when we are within groups. But it's still buggy, the problem seems to me to be with qrDecompose that generates a transformation different from the one we calculated, it messes up all the points. In fact, this would only cause problems when you have skew applied. I have to think better what is happening inside group...

[ShaMan123]

updated from master
I would like the file to be renamed to poly now that it does not contain a poly control.

[ShaMan123]

a6fc8dd fixes logic under group
skew logic should be restored if not since it is a fix (which is more than enough... it is absurd IMO needing to defend this) then because it is a very complex fix and important knowledge virtually no one could reach (fact is no one did for years besides @luizzappa and issues were submitted).

[ShaMan123]

And if not fixing skew, why fix flip or group?

[luizzappa]

a6fc8dd fixes logic under group

nice!

Is there any method to recalculate the BBOX of a group? We need to invoke it here:

fabric.js/src/controls/polyControl.ts

Line 64 in a6fc8dd

poly.setDimensions();

It seems to me that this behavior is because the BBOX is not updated:

[ShaMan123]

Yes there is but this should be handled by group...
I don't understand why it isn't.
The modified event should fire and inform group to perform layout.
During modification it is on the dev (or later on group will optimize it, I have a proposal) to relayout or handle transient caching state.

fabric.js/src/shapes/group.class.ts

Line 348 in 818a134

object[directive]('modified', this.__objectMonitor);

fabric.js/src/shapes/group.class.ts

Lines 315 to 318 in 818a134

	__objectMonitor(opt) {
	this._applyLayoutStrategy({ ...opt, type: 'object_modified' });
	this._set('dirty', true);
	}

The imperative method is triggerLayout but this logic should not invoke it.
Does the modify event trigger at the end of the interaction?

[ShaMan123]

Works as expected.
Group and active objects relationship isn't yet defined properly.
Group might not render the active objects etc.

https___f34gt2.csb.app.-.Google.Chrome.2023-01-07.09-55-20_Trim.mp4

So for now you should cancel caching while modifying

[luizzappa]

So for now you should cancel caching while modifying

Ok, disabling objectCaching worked, but how did you recalculate the BBOX of the group. Here it continues with the initial BBOX:

https://codesandbox.io/s/custom-control-poly-fix-group-2bv3hi?file=/src/index.js

off topic:
This BBOX border of any polygon inside a group is incorrect.. I'll fix it in another PR.

[ShaMan123]

It doesn't work simply because group has to be initialized as follows:

new fabric.Group(objects, { subTargetCheck: true, interactive: true })

I am surprised it worked at all. Ohh it is beause you set the active object. If you comment that line out you won't be able to touch the polygon nested in the group.

Regarding the bbox. Didn't we discuss it in the tests PR? Wasn't it solved? I forgot.

[luizzappa]

Regarding the bbox. Didn't we discuss it in the tests PR? Wasn't it solved? I forgot.

No.. Actually that PR is blocking this fix. I won't include it there so as not to pollute it even more.

[ShaMan123]

It should be simple...
Telling group to layout after setDimensions.
Same as what happens in Object#_set

[asturur]

Everything is moving to classes, why not controls as well?

I get asked the same question over and over again. We can't change all. Migrations have costs.

Can this feature works with the old interface?
Is that really bad that can't be done or is so bad code that is not worth it?

Yes everything is moving to classes and is being painful, it is taking forever, a lot of old code is swapped with new code that is supposed to do the same things but the code has to be different. Is a zero sum game that is not fun to play.

[asturur]

I m trying to run it in code sandbox with the latest code but it doesn't work, i m copy pasting a fresh fabric build from your branch to the codepen but fabric is not defined anymore.
So i m doing something wrong probably

[asturur]

ok, had to force fabric object on the window for some reason on codesandbox.

It works great, it sticks to the current pattern, i m merging it.

[ShaMan123]

@asturur please reconsider skewing!!2@!! We are talking about a number of lines and that is it.

[biggestdickus]

Maybe this might give you some ideas:
https://jsfiddle.net/RobertWStewart/pyqjrfa4/
2D polygon flip, rotate, resize, whatever by just redrawing the polygon.