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 dashed stroke rendering with strokeUniform property #5953

Merged
merged 1 commit into from
Nov 14, 2019
Merged

Fix dashed stroke rendering with strokeUniform property #5953

merged 1 commit into from
Nov 14, 2019

Conversation

korzo89
Copy link
Contributor

@korzo89 korzo89 commented Nov 6, 2019

Fixes issue #5939.

I removed the code lines pointed by @asturur in the following comment:
#5939 (comment)

It seems that the code was added to prevent "ugly" rendering of short dash array distances, however IMHO this should not be a feature of fabric.js - the user should be solely responsible for properly defining dash arrays. The rationale behind this is that unscaled objects should look the same, regardless of the strokeUniform property (see images below).

I used the following code for testing:

canvas.add(new fabric.Rect({
    left: 10,
    top: 10,
    width: 300,
    height: 75,
    fill: '#00ff00',
    stroke: '#000000',
    strokeWidth: 5,
    strokeDashArray: [1, 1],
    strokeUniform: false
}));

canvas.add(new fabric.Rect({
    left: 10,
    top: 120,
    width: 300,
    height: 75,
    fill: '#ffff00',
    stroke: '#000000',
    strokeWidth: 5,
    strokeDashArray: [1, 1],
    strokeUniform: true
}));

For unmodified fabric.js code the test produces the following result:
image
As you can see, the strokes look dramatically different.

After modifying the code, the result looks as follows:
image
The strokes are now identical.

@asturur
Copy link
Member

asturur commented Nov 6, 2019

Hey @stefanhayden do you remember why we decided that in the dash stroke, the stroke width should matter?
I'm sure it counts in our app, but why we added this when we ported back here the strokeUniform?

Reading it today seems unrelated.

@stefanhayden
Copy link
Member

I though it was to fix how it looks as it scales. the whole point of strokeUniform is visual consistency of the stroke as you scale it. I'll have to look how it scales with strokeDashArray with the proposed code but if we loose visual consistency during scaling then we might need a different solution. Clearing the above static example of the current state looks wrong.

@awehring
Copy link

awehring commented Nov 7, 2019

@stefanhayden
You may look at the original issue #5939. I show there how I handle it in my application. Scaling the DashArray linear with the strokeWidth is not pretty.
I'm not shure if this can be handled in a general approach in fabric.js or is better left to the application.

@asturur
Copy link
Member

asturur commented Nov 7, 2019

Yes the point here was consistency.

this is what i would expect from a stroke uniform ( yellow ) and a normal ( green )
gsplit2

While this is what we have now
gsplit2

What we are trying to fix is the bigger dash, in static condition, with the same values of non uniform.

I think a dev would expect that 2 stroke dash with same values would look the same at scale 1

@stefanhayden
Copy link
Member

yeah I agree that @asturur's first image is toally what should happen and if it doesn't then it seem like a bug.

@asturur
Copy link
Member

asturur commented Nov 7, 2019

@korzo89 what are you missing now is a visual test added to close the PR.
Please look in the tests/visuals folder of one to add.
If you need guidance, ask.

@asturur
Copy link
Member

asturur commented Nov 14, 2019

I will add a visual test.

@asturur asturur merged commit 326459a into fabricjs:master Nov 14, 2019
@asturur asturur mentioned this pull request Nov 14, 2019
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
ickata pushed a commit to ickata/fabric.js that referenced this pull request Jul 27, 2021
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.

4 participants