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

fix(#8344): stroke projection #8374

Merged
merged 162 commits into from
Jul 19, 2023
Merged

Conversation

luizzappa
Copy link
Contributor

@luizzappa luizzappa commented Oct 16, 2022

Added tests for #8344

Tests revealed bugs and edge cases that this PR addresses.

The goldens make reading the diff impossible since github boils. View the diff here instead https://github.com/fabricjs/fabric.js/compare/8374-view-diff

@luizzappa
Copy link
Contributor Author

The tests are generating around 3,000 images. It makes sense? Or is it better to define some sets of points beforehand, and just apply scale, skew and miterLimit?

@luizzappa
Copy link
Contributor Author

It seems to me that the goldens were generated by node (which has a problem with miter-limit)

image

@luizzappa luizzappa mentioned this pull request Oct 16, 2022
@ShaMan123
Copy link
Contributor

If we can test all cases with very little tests that is the best

@luizzappa
Copy link
Contributor Author

I reduced the amount of tests to be more targeted. I think it's better this way.

Goldens look like they are being generated by node instead of browser, due to miter-limit issue:

acuteAngle-polygon-20skewX-0skewY

@luizzappa
Copy link
Contributor Author

luizzappa commented Oct 17, 2022

Tests reveal a bug when the strokeLineJoin is bevel or miter and has a straight angle

image

The problem seems to me is that in these cases we only project "outwards", this works for almost all points arrangements, but in the straight angle a straight line is formed, so we need projections to both sides.

image

If we check if the bisector angle is equal to PI, and in those cases project to both sides, we fix that. (not tested yet)

@ShaMan123
Copy link
Contributor

If we check if the bisector angle is equal to PI, and in those cases project to both sides, we fix that. (not tested yet)

Maybe we use line cap projections in the case?

@ShaMan123
Copy link
Contributor

Goldens look like they are being generated by node instead of browser, due to miter-limit issue:

What cmd are you using?

@ShaMan123
Copy link
Contributor

image

make sure you run with -- -c chrome

Resize the goldens. White space is very bad since it is counted as pixels, making the diff% inaccurate.

@ShaMan123
Copy link
Contributor

ShaMan123 commented Oct 17, 2022

I see that is stroke line cap tests the stroke line join is wrong, I am correct?
Does it come from a bug in tests?

@stale stale bot added the stale Issue marked as stale by the stale bot label May 21, 2023
@fabricjs fabricjs deleted a comment from stale bot May 22, 2023
@ShaMan123
Copy link
Contributor

@asturur this is waiting too long

@ShaMan123 ShaMan123 added fix-next and removed stale Issue marked as stale by the stale bot labels May 22, 2023
ShaMan123
ShaMan123 previously approved these changes Jul 18, 2023
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

Dear @luizzappa !
I hope I find you in a good and happy state.
My apologies for not seeing this PR through.
It is very good work and a shame it has been stale for so long.
I have done my best to fast forward it to the current master but I might have done something wrong. I suspect so. But I doubled checked with a view diffs (8374-view-diff...8374-polyline-diff) to make sure but still, your review is priceless.
Once you give a green light I merge it straight away.

src/shapes/polyline.class.ts Outdated Show resolved Hide resolved
@ShaMan123
Copy link
Contributor

I approved, if you have rights to squash and merge you are welcome to do so.

ShaMan123
ShaMan123 previously approved these changes Jul 19, 2023
@ShaMan123 ShaMan123 changed the title test(#8344): stroke projection fix(#8344): stroke projection Jul 19, 2023
@ShaMan123 ShaMan123 merged commit 73fa847 into fabricjs:master Jul 19, 2023
@asturur
Copy link
Member

asturur commented Jul 23, 2023

@luizzappa thanks for bringing this forward.
The reason why was waiting so long is because i m trying to finish the TS transition of the library and write documentation.
I would eventually not merge anything till that is done but @ShaMan123 can't really resist.

The issue with merging things is that we add new things to the library that later with a revise of documentation could entirely stop to make sense and then we would be in the weird position to make changes to code just merged.

I really would love to not add more code while we are in this beta status for so long.

@ShaMan123
Copy link
Contributor

It is a bug fix I need
As many of my PRs, these are things I need

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 23, 2023

I brought it up from the dead

@ShaMan123
Copy link
Contributor

I sort of accept the fact that my prs wait until they are stale, not happy about it.
But I do not think it supports people contributing to the project, the opposite actually

@asturur
Copy link
Member

asturur commented Jul 23, 2023

I know i m not happy everything to be stale either.
I accepted TS and Classes as a way to improve the library, even if i don't fully believe in them, and i m committed to finish it now. As i said 1000 times, adding more stuff while doing it is counter poductive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants