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 resolution scale for globe lighting and labels #8351

Merged
merged 3 commits into from
Nov 6, 2019

Conversation

IanLilleyT
Copy link
Contributor

@IanLilleyT IanLilleyT commented Nov 3, 2019

Fixes #8358

I rushed #8318 and missed some nuances with czm_metersPerPixel that affected globe lighting and labels at different resolution scales. More details about the globe lighting problem here: #8348

This PR fixes those problems. I also refactored the addScreenSpaceOffset function inside BillboardCollectionVS.glsl. Now all resolution scaling is handled in the shader, not the js file, for no extra cost. I tested the labels and billboards sandcastles thoroughly as well as some others, comparing against 1.60 and 1.62 releases. It seems in working order, but worth a double check.

@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.

@OmarShehata
Copy link
Contributor

This came up on the forum here. I can confirm this fixes the issue.

In 1.63:

image

In 1.62 and in this branch:

image

Code:

var viewer = new Cesium.Viewer('cesiumContainer', {
    imageryProvider : new Cesium.TileMapServiceImageryProvider({
        url : Cesium.buildModuleUrl('Assets/Textures/NaturalEarthII')
    }),
    baseLayerPicker : false,
    geocoder : false
});

viewer.entities.add({
    position: new Cesium.Cartesian3(1216361.4096947117, -4736253.175342511, 4081267.4865667094),
    label: {
        text: "The quick brown fox jumped over something I can't remember."
    }
});

@lilleyse
Copy link
Contributor

lilleyse commented Nov 5, 2019

@IanLilleyT add the two fixes to CHANGES.md

mpp = czm_metersPerPixel(positionEC);
positionEC.xy += (originTranslate + halfSize) * (sizeInMeters ? 1.0 : mpp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
positionEC.xy += (originTranslate + halfSize) * (sizeInMeters ? 1.0 : mpp);
positionEC.xy += (originTranslate + halfSize) * (czm_branchFreeTernary(sizeInMeters, 1.0, mpp));

@lilleyse
Copy link
Contributor

lilleyse commented Nov 5, 2019

I tested a bunch of Sandcastle examples. I also checked that Label.getScreenSpaceBoundingBox and label.computeScreenSpacePosition return the same values as master. All versions return the same value regardless of resolution scale, which means the label/billboard/clustering code is operating in CSS pixels as it should. Aside from fixing the regression the code is a lot cleaner - GLSL code went from 18 (confusing) lines down to 3. And more resolution scale code is moving to the shader where it belongs.

@IanLilleyT is going to write up an issue for testing device pixel ratio changes so problems can be caught by CI rather than manual Sandcastle checks.

@mramato
Copy link
Contributor

mramato commented Nov 5, 2019

Given that this issue is much more widespread than originally though, we should do a 1.63.1 release after merging this. As part of the release I will test all Sandcastle examples on a high-dpi machine and we will have to address any additional issues that might come up (hopefully none).

@lilleyse Any reason this can't be merged early tomorrow so we can go through the release process?

@lilleyse
Copy link
Contributor

lilleyse commented Nov 6, 2019

Looks good to me. @mramato thanks for doing the patch release tomorrow.

@lilleyse lilleyse merged commit fb7dbcf into master Nov 6, 2019
@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/6yHrumlfuWU

If this issue affects any of these threads, please post a comment like the following:

The issue at #8351 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

@lilleyse lilleyse deleted the metersPerPixelFixes branch November 6, 2019 03:37
@mramato
Copy link
Contributor

mramato commented Nov 6, 2019

I'm still seeing difference in label quality with different DPR values.

DPR 1.0

image

Same machine, DPR 3.0

image

The screenshot doesn't do it justice, but hopefully you can see the difference. Am I correct in that the goal is to have different DPR values have zero effect on rendering? You can test this yourself using the chrome responsive debugger.

@mramato
Copy link
Contributor

mramato commented Nov 6, 2019

Actually, click on each image and then swap the tabs back and forth and it's pretty obvious.

@mramato
Copy link
Contributor

mramato commented Nov 6, 2019

My assumption of the above is that since I'm testing on a DPR 1.0 screen and manually setting DPR to 3.0, it's actually rendering the text at a higher resolution and then downscaling, accidentally improving overall visual quality. This is probably okay, but I would like someone who knows more about the recent label changes to confirm the exact behavior so we aren't making assumptions.

@liyangis
Copy link

liyangis commented Nov 7, 2019

when the label'text is in chinese,when i zoom in use mouse,it will be
Wrong line displayed, cesium 1.62,it is in the demo called " clamp to Terrain" in Sandcastle .
QQ截图20191107160030
QQ截图20191107160053

@IanLilleyT
Copy link
Contributor Author

@liyangis could you create a new issue with a sandcastle that shows the problem?

@lilleyse
Copy link
Contributor

lilleyse commented Nov 7, 2019

@liyangis I think that may be a duplicate of #7376

@liyangis
Copy link

liyangis commented Nov 8, 2019

@liyangis could you create a new issue with a sandcastle that shows the problem?

https://sandcastle.cesium.com/#c=7Vrvbts4En8VXr9YRl1Gfy2pTYpzErcbrGMHjrdF0RQFbdE2b2XRoOik6SLvsl/uMfZt7t7jhqLkyLKcP9umtx9WCGBLmhmSw5nf/IbxJRHoktErKtABSugVOqIpWy3wu+yZ0Zhkt0c8kYQlVDRa6LeLBMElqRDw6EzwSxZR8bJQnAhKJH3PRRyNtIjRvEhumq8uEj0OTic0oXgW8zHFEV3K+YimsjMDyVTmKjAXKVYUdC6Sc5JEE5LKmGISRcd0SlaxHHEej4k4pcnK+JjPaG+v+EQjjiYxWSzRkrNEpogLNGZxPOZERClKqURyTtGcstlcDumUCppMKJIcHfU6p2efR4PPb4eDX/rHSnHY7XVGJ++6t083h5P0i0QvUeNYkCt0psZDV0zOUY+MadxoaSGepDSmEyU4XSUTyTi4pXClui5hHygsO/cRTSSTjKZqyUZJTF1LnjJlAK19fkSEhG8kcfBU8MUxnQlKU+OFZdvYCr2ghdw2fLG8ZqtqSk33JaqMoK4Jj2Hxt0OoW3z+84fD3i/d1rb4kn2h8Tn7SkHFMmsE+ErGEEFHdWY/dHu9wfvdSu9ZBA59iZwakeomrk3/tPkCV7Z209JNxXKsNq/eMcV+H6kAo5GKmpngqyS6ANk5jWOuvvznj9//++8/GjUTnvLM5Q3LXUqUkiR9kVLBpo3vuLY6U1ywryqL44FgM1YKnp8qb3Cv+2ZUY+GSCskm2/rvNp7jw855t3fSr4uRKaRg7f4f9jpHP9copHN+dUgmv2r/gpYChRq58VqmMF9CsuyRYbWQ/jOx37zTxBmkHEtmFSNFhtmGyqY6CxFLyTimxwWiHbNUEr1r/dViDEl9Njg/yZDkpP/mpH8y+qCgKgLYSDM40jEHroUchhDh0wJjK6F6e5uhaglDNHZIAWuhUVdByDVAioLRXPGmBN8l1DossPEvDldrDK/PTLYgM+XuBsZ72fd0b0omLAY34Nn3zbCn2JJS/TriQrCIi7QF7ouvZ7AfCMogErAnJJnFNIUaE8doTHWdAxgiK8kXJMvE+BqxqQqpa0QEzdIOBLKiRADXExWXMgd4sLoedU5SlPDcJ6r0QYyIVQS6+hFeS/YHo+5LdE6lVJmSKwAQmnpaeSrouYEEvqNiFkt9itCb5Lbrw6UIzPS+yOxAFl4bH7ctqCuP2iJm3dYdYraVicHHXUJOLrQt86kOd67y4mibponrKi9EBRQYEldR9+2w2+1jFRadeDknhom95iPj+ivnixE3aPNeiDnTYfw0AJOZrt/kOaOCiMn8uv51NQ4+1mG+Y7ywHS+wnAAHru+4ph3ARr5wfNc1fRu7Xui5FtzBvoGU5QXY8y3X8j3H95o7tnrj2j2obzsWdkMbTJlt09GjepbpYD8I2m3TtgIvG9b0TRuHvuf7bbvtfOuwXmiBNdO2Tc91irXabohNO/T9wPX0Wm1wBbZAxA9gSt+8VDMA+2boebCCth4UpuJgGNp0g8AqPBy2LRy0HRscYIfNT9uj3jwiCxSf/VFJMIKblQA8fWw2AJwb/8inrYsITlfLJRcyPc3XlU656OZZMkiK1qvccjWb1Qy4YknErzCJgcIZjaL3yqtU4bA0KyIJB7Kqh4T5Q7bJOUvRMiYSxl00irUXl6Cw0KT08GbDcX8n99/J/UTJXSJ/eiqfe3zGP+tk/9dyVscCgaekKZsCd1KhMrpelojg0dY7POoOh52Tfl3DIodckk22Cwk6x5IPSQTeSA33CfFlWNDDpyiza+6567yAC+ibYCNKfGo9oW2ir1mO2dKkx8zuaqNsF3APu8c/CrdPefSXP8pZqDnWb81KsDwxzoGPQ4tKJNnL5IscOSVJkS3wFc/i8RMfRyxYwharxVn51MgOanf/ixI8h7ZGHy2ZT9kNa/8gdeZUqhyq81Jtum6hsiNL0Ehl0ec8rm2OaTIDAwdqLearalmE3kzIHlcnoPX4EbRxCAUjdMolNwu5JLpHLwjbbug3t8aEIr5TzfZxGAS2V6OWn05on52tvXWAPn4qTQ3oATKUOIM35iv42M9d8Ao9f862OEk2oa11AENZGnqFrbWPWmBsDxm5Q18gq1mlIcrYOscOqvWJzwRZztnEiJUt8EJVvX6FeLlK50ZhtrmL5OQDXc1pYuTf08xQzrNOeSqPqSQMAqjgaZVD9daOGTRbt/Glbaa1fuTTqTrrVqGGzcra6vYlt4Xv2p+szGm5j+wTznv/5wf5YK+20rMyrftBsaB9WRY+iNgVzDiO2TLlLMLv354HLp6U9jjr4Ud8Da+6py+ct4PcALfcJAMd/QD3B7UHnOV23NvxvlTOSuF4lq93oA+8C1IPgbAEbn5t/LabfdUe1A+Gnf7b7h2crXKybt8vuvvktl71ZpdXMwB9A3F/+kNcATzh/+yHGg5byZJNcF0DLBGzLH3rebVjmr4PvMcMoJEIQicABu21HVfRe8uxnBDoNPQPQOoDy7MxMGrPg/bCaldhbg0T9eOASRe6D9cMg7YfeG0bOBvUH+y4VuiHgam4O7QR0EGEUJpMaF1Mv+1YbXsLjXXuT8gC2jYcc/5rRxp6ka18Eg9RGQkoTKrrNG4rhGBfXHxy3O2PTkYfNiD5fna3xhrV1Wq8vbegP6g1LyL5T/bja/XSxB7bi9/UMKL72u3duPt9TkzXDAZiBtiFrdhF60Giwd2imt+URB94gpodU4/423v+y3QXsn9XVH88oj8Qxf4Egt08vKv6QYi1QWyeELEeiVbfhFQKnD61UEO1pIcrKSHFGs3qrx8E1WsukAjd9hbVzBZ0wS9pJ44NZeUms/Ss9Ww/ldcxfa11/skWCkGgO4wNaA0lXSgEoeneeAUNk8STNFXK+3uF0n7ELhGLDi6eVX4UcvFMn6DAm+kqzvq5i2ev9/dAfkMt5kT9Z3VwSUVMrpXI3Hrd0w8xxvt7cLutJfVPPUBcz7v0JifSNFmVxtuUKXl0pwyQxFlMe4rIwkx2immyuG2s+Pwf

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.

resolutionScale related lighting issue
6 participants