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

Fixed pixel size for polylines and point clouds under different resolutions #8227

Merged
merged 2 commits into from
Oct 1, 2019

Conversation

IanLilleyT
Copy link
Contributor

@IanLilleyT IanLilleyT commented Sep 30, 2019

This is a partial fix for #8113

Polylines and point clouds had a number of areas that were not consistently sized between different resolutions:

  • Point cloud size
  • Styled point cloud size
  • Point cloud eye dome lighting radius
  • Polyline arrow
  • Polyline dash
  • Polyline shadow volume

Point cloud
new (low res):
point_lowres
new (high res):
point_highres
cesium 1.60 (low res):
point_160

Polylines
new (low res):
poly_lowres
new (high res):
poly_highres
cesium 1.60 (low res):
poly_160

Further thoughts:

  • I tried to move the pixel ratio multiply outside shaders where possible, but some systems are not flexible enough for this. See PolylineShadowVolumeMorphVS.glsl, which "spawns" the czm_batchTable_width function prior to shader compilation, where there is no easy way to insert the pixel ratio multiply. getPointSizeFromStyle in PointCloud.js has the same problem.
  • Polylines which use the renderState's lineWidth property still look incorrect. This is because they are rendered as GL lines which is capped to 1 pixel on many computers. Example below:

Native resolution:
Screen Shot 2019-09-30 at 9 51 33 AM
Browser recommended resolution:
Screen Shot 2019-09-30 at 9 51 44 AM

@IanLilleyT IanLilleyT requested a review from lilleyse September 30, 2019 15:54
@cesium-concierge
Copy link

Thanks for the pull request @IanLilleyT!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@mramato
Copy link
Contributor

mramato commented Sep 30, 2019

Tagged this for next release since I think it's important to get out and is a regression, @lilleyse I assume you'll be able to review/merge this today?

@lilleyse
Copy link
Contributor

@mramato Yes I'll review today

@lilleyse
Copy link
Contributor

For the lineWidth example I am noticing a difference on Linux where there isn't the 1px limit.

Screenshot_2019-09-30_17-10-51

@IanLilleyT
Copy link
Contributor Author

Ok, I added that to the checklist in #8113

@lilleyse
Copy link
Contributor

lilleyse commented Oct 1, 2019

Thanks @IanLilleyT , these fixes look good. Looking forward to the next batch in #8113 (after the release)

@lilleyse lilleyse merged commit 019ae68 into master Oct 1, 2019
@lilleyse lilleyse deleted the cssPixelFix branch October 1, 2019 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants