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

Prototype pixel snapping icons with GL_LINEAR #8738

Closed
wants to merge 1 commit into from
Closed

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Sep 8, 2019

Implements #8730.

We're currently switching between pixel-grid aligned icons in a binary way. In a follow-up patch, we could implement a gradual switch by making u_crisp a float, and scaling it between 0 and 1 whenever the user interacts with the map.

  • More visual tests
  • Verify that it works correctly at other pixel ratios. I've observed that neighboring icons from the image atlas bleed into the icon at the sides and we may potentially have to scale the 1 px border with the pixel ratio (i.e. 2 pixels) to account for the pixel grid alignment.
  • Verify on a large number of devices. I already tested a few different ones and they all work correctly. However, since this relies on floating point precision and rounding, there's a chance that some devices may behave erratically. I tried to take this into account when designing the calculation by using the projected center point, before the extrusion vectors are applied to get identical results for all four vertices of the icon.
  • Potentially rework the way we use the transformation matrices to reduce the number of calculations we have to do; we currently have 3 4-dimensional projection matrices in the shader and I'm sure we can reduce that a bit. The code currently uses the canvas size to "unproject" from GL space (-1...1) to pixel space so that we can obtain the fractional pixel value to shift the texture coordinates.

@kkaefer kkaefer requested a review from ansis September 8, 2019 13:08
vec2 result = fract(value);
if (result.x > 0.5) result.x -= 1.0;
if (result.y > 0.5) result.y -= 1.0;
return result;
Copy link
Contributor

@astojilj astojilj Sep 26, 2019

Choose a reason for hiding this comment

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

Shorter way to write it return fract(value) - vec2(greaterThan(value, vec2(0.5)));. Shouldn't matter, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can try, but many OpenGL implementations I tested are very picky about subtle changes like this and perform rounding differently.

v_tex = a_tex / u_texsize;
v_tex = a_tex;
if (u_crisp && (segment_angle + symbol_rotation == 0.0)) {
// move texture loopup to snap to the pixel grid
Copy link
Contributor

Choose a reason for hiding this comment

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

loopup/lookup

// move texture loopup to snap to the pixel grid
vec4 icon_pos = u_coord_matrix * vec4(projected_pos.xy / projected_pos.w, 0.0, 1.0);
v_tex += absfract((icon_pos.xy + 1.0) * u_canvas_size * vec2(1.0, -1.0) / 2.0);
v_tex += fract(a_offset / 32.0 * fontScale * u_device_pixel_ratio);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to multiply this by u_coord_matrix before fract, as it is in gl_Position calculation?

@kkaefer
Copy link
Member Author

kkaefer commented Oct 14, 2019

Related issue: #8110 (and #8118)

@kkaefer kkaefer added this to the release-b milestone Jan 27, 2020
@kkaefer kkaefer modified the milestones: release-baltic, release-c Feb 12, 2020
@hrach
Copy link

hrach commented Feb 24, 2020

Is this going to be fixed in release-c? Really looking for seing this fixed :) Thanks.

@ryanhamley ryanhamley removed this from the release-c milestone Mar 10, 2020
@asheemmamoowala asheemmamoowala marked this pull request as draft April 10, 2020 01:00
@asheemmamoowala asheemmamoowala linked an issue Apr 10, 2020 that may be closed by this pull request
@arindam1993 arindam1993 changed the base branch from master to main June 18, 2020 18:13
@ryanhamley
Copy link
Contributor

Closing this as stale

@ryanhamley ryanhamley closed this Jan 21, 2022
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.

Always use GL_LINEAR interpolation for icon rendering
4 participants