Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(V6): Group Rewrite + Nested Selection #7669

Closed
wants to merge 173 commits into from
Closed

feat(V6): Group Rewrite + Nested Selection #7669

wants to merge 173 commits into from

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Feb 10, 2022

Group Rewrite

This PR has been broken down into #7858 #7859 #7860 and will be left open for tracking only.

This description is updated continuously with relevant information
You don't need to scroll and read endless comments (unless you are a maintainer :))

Motivation

#7473 #7316 #6776 #7136 #7299 #7130 #7142 #7449 and anything with a group tag

closes #6499, closes #3012, closes #3000, closes #7130, closes #7142, closes #7598

Nested selection is a MUST.
Group wasn't easy to use and was very limiting (e.g. doesn't support resizing logic, customize rendering - bg) and confusing (relative coordinate system and transform).

Current State

Fixed logic across fabric for nested objects to be selectable.
Selection logic needs refactoring to support some cases of nested selection (multiselect), for now use canvas.setActiveObject.
Group has been completely rewritten and Layer has been introduced. As part of the rewrite group is now a layout engine, you can use it to customize layout of objects.
Group now respects originX originY (still buggy)

BREAKING

  • all awkward methods such as addWithUpdate are removed

Working Examples

POC

Tryouts (out of scope, and stale, here to light the way):

Alpha testing

To Do

  • refactor selection logic (use dblClick to select nested objects? create mechanism for dev to customize selection)
  • refactor to enable n levels of nesting (logic is adequate for 2)
    // in case a single object is selected render it's entire above the other objects
  • @asturur rethink preserveObjectStacking in favor of a drag image/ghost
  • text selection in scaled and rotated group is wrong (getLocalPointer 8bf7f92)
  • bounding box and controls of object under transformed group (fix(v6!): nested controls #7758 )
  • phase out coords (can be delayed, now that bbox and controls are fixed)
  • look into active selection - creating from nested objects
  • update tests
  • object caching in some cases leaves traces
  • refactor object caching
  • caching edge case - nested absolute clip path (see fiddle, try moving group, image should be clipped). Possible fix is to handle a registry of children with abs clip paths OR modify the shouldCache method.
  • fix group's caching behavior + shadow logic - Group With ClipPath and Text with Shadow Renders Incorrectly #7454
  • support layers under groups?
  • ActiveSelection.toGroup use removeAll instead
  • getObjectsBoundingBox - needs work to exactly fit when group is rotated
  • initial layout inconsistent with old group
  • initial layout - supporting originX/Y 407023c and angle
  • groupSVGElements
  • support fill, stroke
  • from svg - parsing back fill/stroke without adding redundant rect, layout props
  • rethink _renderObjects - nested active object will fail with perPixelTargetFind because it will not get rendered by group (add logic to canvas.isTargetTransparent)

Look

Video is running in original speed
tiger

Cutting Edge

The blue circles are rendered by a textbox and because of the rewrite with a few lines of code I got this working
Pay no attention to caching of course

This is EXTREMELY exciting because it means that anyone could create custom objects subclassing whatever object they need and have nested selection work out of the box!

ezgif com-gif-maker (13)

@ShaMan123 ShaMan123 changed the title V6 feat(V6): Group Rewrite Feb 10, 2022
@ShaMan123 ShaMan123 changed the title feat(V6): Group Rewrite feat(V6): Group Rewrite Feb 10, 2022
@ShaMan123 ShaMan123 mentioned this pull request Feb 10, 2022
48 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

Code Coverage Summary

> fabric@5.0.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |    77.22 |    86.2 |   83.04 |                                               
 fabric.js |   83.31 |    77.22 |    86.2 |   83.04 | ...,30257,30382,30462-30527,30650,30749,30966 
-----------|---------|----------|---------|---------|-----------------------------------------------

moved to object_origin mixin
@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

Code Coverage Summary

> fabric@5.0.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |    77.24 |   86.14 |   83.03 |                                               
 fabric.js |   83.31 |    77.24 |   86.14 |   83.03 | ...,30247,30372,30452-30517,30640,30739,30956 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

Code Coverage Summary

> fabric@5.0.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |    77.26 |   86.14 |   83.03 |                                               
 fabric.js |   83.31 |    77.26 |   86.14 |   83.03 | ...,30247,30372,30452-30517,30640,30739,30956 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Feb 11, 2022

Code Coverage Summary

> fabric@5.0.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |    77.24 |   86.14 |   83.03 |                                               
 fabric.js |   83.31 |    77.24 |   86.14 |   83.03 | ...,30247,30372,30452-30517,30640,30739,30956 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 requested a review from asturur February 12, 2022 01:53
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 28, 2022

I have dropped old group's caching checks so the issue with caching probably lies there.
I didn't build new logic because, as we discussed, there's a need to rewrite it.

* Remove objects
* @param {...fabric.Object} objects
*/
remove: function () {
Copy link
Contributor Author

@ShaMan123 ShaMan123 Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add relativeRemove or some name for a function that removes objects without removing the group's transform

if (object.group === this) {
throw new Error('fabric.Group: duplicate objects are not supported inside group');
}
object.group.remove(object);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I think it makes sense to preserve the old group's transform if removeParentTransform is true

@ShaMan123 ShaMan123 linked an issue Mar 1, 2022 that may be closed by this pull request
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Mar 3, 2022

Alright,
Future thoughts regarding new selection logic:
If an object is selected, logic target checking should bubble up the tree to parents, siblings and relations in some manner.
Only after bubbling has reached it's course and is now at the top most ancestor of the active object should it iterate down the tree on all remaining objects searching for a new target to select.

Thoughts? @asturur @melchiar

@ShaMan123
Copy link
Contributor Author

@asturur I now understand your incline not to have fill/stroke for group. I was not aware of backgroundColor which is good enough for most use cases. So I don't oppose removing

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Mar 14, 2022

Take a look at THIS!

The blue circles are rendered by a textbox and because of the rewrite with a few lines of code I got this working
Pay no notice to caching of course

This is EXTREMELY exciting because it means that anyone could create custom objects subclassing whatever object they need and have nested selection work out of the box! All you need is to reference the parent object as the group property (will rename that I guess) and reference the object in _objects array of parent

Custom classes will become much easier to build

ezgif com-gif-maker (13)

@ShaMan123
Copy link
Contributor Author

This PR has been broken down into #7858 #7859 #7860 and will be left open for tracking only

* Update object_geometry.mixin.js

* fix(): selection controls connecting lines

* Update object.class.js

* Update object_geometry.mixin.js

* fix(): flipX control

* tests

* lint

* Update control_rendering.js

* Update active_selection.class.js

* flipX

* tests(): flipX

* Revert "tests(): flipX"

This reverts commit 58e2cd5.

* Revert "flipX"

This reverts commit 5e129d1.
@ShaMan123 ShaMan123 marked this pull request as draft April 4, 2022 19:03
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Apr 4, 2022

apart from _renderObjects this PR has been completely consumed

@ShaMan123 ShaMan123 closed this Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment