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 line-pattern sampling from outside icon tex coords #6246

Merged
merged 5 commits into from
Mar 6, 2018

Conversation

mollymerp
Copy link
Contributor

@mollymerp mollymerp commented Feb 28, 2018

fixes #5978

not sure if we should repeat the pattern instead of stretching 💭

test on master:
actual

this branch:

actual

updated with render test after d78d242

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mollymerp
Copy link
Contributor Author

mollymerp commented Feb 28, 2018

hmm whoops – this is not the correct fix. it is flipping the icon, and (flipping was existing behavior) not stretching the whole pattern (duh 🤦‍♀️ )
expected

@mollymerp
Copy link
Contributor Author

Ok, I think the fix is correct now. Added some explanatory comments as well.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Deferring to @ansis here.

@ansis
Copy link
Contributor

ansis commented Mar 2, 2018

Hmm, another option besides repeating and stretching would be just to draw nothing in the extra space. So if you have a 24px pattern and a 36px wide line it would look the same as a 24px pattern and 24px line.

I'm not sure which would be preferable, I guess it depends on the pattern?

@mollymerp
Copy link
Contributor Author

mm good point. @aparlato @nickidlugash any opinions on desired behavior for line patterns with width > pattern height?

@aparlato
Copy link

aparlato commented Mar 2, 2018

mm good point. @aparlato @nickidlugash any opinions on desired behavior for line patterns with width > pattern height?

Oy this is a tough call. I'm inclined to say stretching meets more needs overall? The instances that I have used this property, this was the behavior I wanted. Although, @mollymerp, it looks like this only stretches it by width rather than treating the whole icon as larger. What's the reason behind just stretching by width rather than maintaining aspect ratio?

Hmm, another option besides repeating and stretching would be just to draw nothing in the extra space. So if you have a 24px pattern and a 36px wide line it would look the same as a 24px pattern and 24px line.

@ansis I think this makes sense in many cases, but since this can also be accomplished by just maintaining a 24px line, I think from a styling perspective this is less flexible unless I've misunderstood the comment.

@ansis
Copy link
Contributor

ansis commented Mar 6, 2018

Oy this is a tough call. I'm inclined to say stretching meets more needs overall?

Sounds good, let's do this.

it looks like this only stretches it by width rather than treating the whole icon as larger. What's the reason behind just stretching by width rather than maintaining aspect ratio?

Scaling horizontally is a bit problematic. If the pattern is scaled at anything other than the rate the line is scaled at then the pattern will look like it is sliding along the line.

@ansis I think this makes sense in many cases, but since this can also be accomplished by just maintaining a 24px line, I think from a styling perspective this is less flexible unless I've misunderstood the comment.

Yep, that's a good point.

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -75,7 +75,7 @@ function drawLineTile(program, painter, tile, bucket, layer, coord, programConfi
imagePosB = painter.imageManager.getPattern(image.to);
if (!imagePosA || !imagePosB) return;

gl.uniform2f(program.uniforms.u_pattern_size_a, imagePosA.displaySize[0] * image.fromScale / tileRatio, imagePosB.displaySize[1]);
gl.uniform2f(program.uniforms.u_pattern_size_a, imagePosA.displaySize[0] * image.fromScale / tileRatio, imagePosA.displaySize[1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

@mollymerp mollymerp merged commit 4b1195a into master Mar 6, 2018
@mollymerp mollymerp deleted the fix-5978-merp branch March 6, 2018 18:49
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.

Line-pattern rendering pieces of other sprites
4 participants