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

Ensure mouse corners match custom control corner size #6562

Merged

Conversation

gloriousjob
Copy link
Contributor

@gloriousjob gloriousjob commented Aug 31, 2020

Pertains to issue 6556. I've added code to ensure that the mouse will highlight over all parts of the custom corner size instead of the original corner size.
I also added a test based on just changing the size of one corner.

I'm not sure if more should be done for the touch code, as I'm unfamiliar with it.

This has morphed to also provide support for rectangular custom controls (a feature desired for 4.0 that didn't make it).
Closes #6556

@asturur
Copy link
Member

asturur commented Sep 2, 2020

we are going to talk about this, i m just busy right now.

@@ -88,24 +88,32 @@
x, y;

for (var control in coords) {
// handle custom control corner sizes
var controlObject = fabric.Object.prototype.controls[control];
var cornerHyp = controlObject.cornerSize
Copy link
Member

Choose a reason for hiding this comment

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

what about extending the controls to return their own corner shape? is a bit more complicated but would solve the problem of both different sized controls and corners that are rectangular.

controlObject.cornerSizeX and cornerSizeY and would also change the math down to something i can't calculate now on the top of my head. But definitely doable

@gloriousjob
Copy link
Contributor Author

@asturur What do you think about this? I wasn't sure how if you wanted cornerSize vs cornerSizeX, cornerSizeY. I also added touchCornerSizeX,Y. More still needs to be done but wanted your thoughts on which direction to take if you want to change some things.

@asturur
Copy link
Member

asturur commented Sep 23, 2020

Hey sorry for not coming back.
I need this exact same feature, and i m a bit short of time.
I want to be sure this PR fix your use case and my use case and i m trying to get the chance to do this at work.

@gloriousjob
Copy link
Contributor Author

gloriousjob commented Sep 24, 2020 via email

@asturur
Copy link
Member

asturur commented Sep 26, 2020

The idea looks good completely.
I m not entirely sure if we want to keep that function in the corner or in the canvas, it does not matter much.
Did you had a chance to try if the calculations are correct?

@gloriousjob
Copy link
Contributor Author

@asturur What do you mean by "try if the calculations are correct"? I adjusted the sizes and noticed that the mouse was properly changing the cursor when within the rectangle. I'm not sure if you mean just verified that things were working? I used basic trig to make the calculations.
I also did a recent update when I found that doing the custom polygon controls wasn't working with this update but looks good now.

br: {
x: centerX + cosHalfOffset,
y: centerY + sinHalfOffset,
},
Copy link
Member

Choose a reason for hiding this comment

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

this bunch of numbers here i remember they work with square and are a simplified version. Once the object is rotated i believe with a strongly rectangular controls, this does not work.
This same thing overly complicated the padding logic elsewhere.

the x and the y coordiante of each point when rotated are both affected by cos and sin of the 2 offsets.

Copy link
Member

Choose a reason for hiding this comment

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

There should be some debug code to uncomment to see those points on the canvas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think I understand the original code now. It's using the complementary angle to triangulate the other corner (which both angles are 45 when in a square, which is why stuff works but it's almost hard-coding to run the original way). Things were working with right angles when I'd rotate so thanks for challenging me to check other rotations.

I'm not sure about debug code though. I did find things below a getImageLines call that I was able to uncomment and see some squares show up so that was hepful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asturur Is there anything else you'd like to see here? I tried adding padding to a shape and the rectangular corner worked, even when rotating.

src/control.class.js Outdated Show resolved Hide resolved
@asturur
Copy link
Member

asturur commented Oct 11, 2020

Testing this now and trying to merge it.
I think that we can dramatically reduce the complexity somehow but will be a follow up.

@asturur
Copy link
Member

asturur commented Oct 11, 2020

image

There is some issue with rendering changing only some controls.
Trying to verify what it is, seems a mutability problem somehow.

@asturur
Copy link
Member

asturur commented Oct 11, 2020

Changes i applied:

  • removed the name cornerSizeX/Y for sizeX/Y and similar for the touch version. Those are controls, corner is an old name and since those are positioned freely, it does not make sense anymore to reference to those as corners.
  • fixed the mutability bug for styleOverride
  • made the circle drawing control one level less nested

@asturur asturur merged commit d4f672a into fabricjs:master Oct 11, 2020
@gloriousjob
Copy link
Contributor Author

I like the rename... I was concerned about it but didn't want to change things without your approval so glad that's good!
I'm not sure I get the styleOverride thing but if things are working, that's what's important (probably because of the fabricObject addition when trying to get the value of the x and y sizes).

@asturur asturur mentioned this pull request Dec 23, 2020
@gloriousjob gloriousjob deleted the ISSUE-6556-custom-control-cornersize-support branch December 24, 2020 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor range on custom control does not extend to cornerSize of control
2 participants