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

Whole new way of computing a polyline arrow interior. #8793

Closed
wants to merge 11 commits into from

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Apr 23, 2020

How did I get roped into this? It has nothing to do with anything I'm working on.

Fixes #3491

Possible continued crazy ideas:

  • Outlines (color + width) would be a good idea to add to this, and would make the arrow head and line pop a little better. Also given this new method, the outline should be able to hold a constant width even around the arrow head and interface with the rest of the line, the same way that fuzz does now. Previously there was a sharply aliased boundary between the head and the line, which is fixed here.
  • Would be nice someday to add a uniform to control headLength
  • Would be nice someday to add a uniform to control centerAmount which I guess should be renamed to centerWidth or some such.

Don't try to read this as a line-by-line diff. The whole body has been replaced by new math.

@cesium-concierge
Copy link

Thanks for the pull request @emackey!

  • ✔️ 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 Apr 23, 2020

How did I get roped into this? It has nothing to do with anything I'm working on.

😆 I guess I'm just that charismatic.

@emackey
Copy link
Contributor Author

emackey commented Apr 23, 2020

outlineColor and outlineWidth have been added to the uniforms at the primitive level.

There's a default 1-pixel width black outline. The previous arrow had an unintentional dark blur around it, on account of blending against transparent black. So, the new default looks somewhat similar to the previous effect, except it's crisp and controllable by uniforms.

@emackey
Copy link
Contributor Author

emackey commented Apr 24, 2020

Since outlines work, you can now do hollow arrows.

var viewer = new Cesium.Viewer("cesiumContainer");
var scene = viewer.scene;
var polylines = scene.primitives.add(new Cesium.PolylineCollection());

polylines.add({
  positions: Cesium.Cartesian3.fromDegreesArrayHeights([
    -120.0,
    35.0,
    500000,
    -80.0,
    35.0,
    500000,
  ]),
  width: 80.0,
  material: Cesium.Material.fromType(
    Cesium.Material.PolylineArrowType,{
      color: Cesium.Color.fromAlpha(Cesium.Color.GOLD, 0.3),
      outlineColor: Cesium.Color.GOLD,
      outlineWidth: 4,
    }
  ),
});

@mramato
Copy link
Contributor

mramato commented Apr 24, 2020

Looks awesome @emackey the lines are definitely much nicer and crisper.

There are some issues with horizon views and ending up with an arrow head on each side, from your Sandcastle:

Peek 2020-04-23 20-55

@emackey
Copy link
Contributor Author

emackey commented Apr 24, 2020

Yep, I believe those are known issues with the way polylines generate their texture coordinates.

@emackey
Copy link
Contributor Author

emackey commented Apr 24, 2020

Or maybe it's an artifact of the line being too short for its own head...

@emackey
Copy link
Contributor Author

emackey commented Apr 24, 2020

Well it's... less terrible? CI still running.

@emackey emackey marked this pull request as ready for review April 24, 2020 01:16
@mramato
Copy link
Contributor

mramato commented Apr 24, 2020

I’m not seeing the arrow at all on my iPad. I can try desktop Safari tomorrow.

@emackey
Copy link
Contributor Author

emackey commented Apr 24, 2020

The arrow worked on my kids' iPad (Pro or something? 2016-era iPad, I can't keep track of their generations). More testing always welcome.

@mramato
Copy link
Contributor

mramato commented Apr 24, 2020

Weird. I tried both my iPhone Xs running 13.4.1 (model MTAL2LL/A) and iPad Pro (10.5 inch) running 13.4.1 (model MQDT2LL/A)

Can anyone else with an iPad/iPhone give a try and confirm if they see the arrow?

@emackey
Copy link
Contributor Author

emackey commented May 5, 2020

@shunter Any chance you can test this demo on your iPhone or other iOS gear?

It should look like this:

ThickArrow

@emackey
Copy link
Contributor Author

emackey commented May 5, 2020

For comparison, here's a similar demo in master (for mobile: standalone demo) with the blurry solid-color arrow.

@shunter
Copy link
Contributor

shunter commented May 5, 2020

No polyline at all. iPad Air 2 and iPhone 7. I checked the console and there were no related errors (some 404s about missing JS source map files)

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@emackey
Copy link
Contributor Author

emackey commented Jul 5, 2020

I'm at a stalemate on this. I presume it's some issue between GLSL and older iOS, but it could be the branch, and I have no hardware that can reproduce it (we have 3 iPads here circa 2017, all show the arrow).

Question: How does the affected hardware handle the arrow in the master branch currently?

If it's not salvageable, feel free to close this.

@mramato
Copy link
Contributor

mramato commented Jul 6, 2020

I believe the next version of Safari is getting a new angle-based WebGL backend. So I'm okay with just leaving this open and revisiting as soon as that update is out. I appreciate the effort either way @emackey!

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@emackey
Copy link
Contributor Author

emackey commented Aug 11, 2020

Merged master in, just in case #9064 fixed anything here.

@mramato
Copy link
Contributor

mramato commented Aug 11, 2020

Thanks @emackey! Unfortunately not. But I still hold out hope the next version of Safari will (since there's a big WebGL overhaul).

I also wonder if there is some undiscovered bug in Cesium that is causing this, (vs the WebGL implementation)

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

4 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

4 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@mramato
Copy link
Contributor

mramato commented Jun 4, 2022

I was curious about this PR since Safari is now using ANGLE and has much better WebGL support. I merged in main and the arrow now shows up on iOS. So aspects of this PR is a huge improvement over what's in main. There are still some weird behavior at certain angles that someone might look into, but I feel like this PR is something we can revisit sometimes soon.

@nikitakogan
Copy link

try to use it on 2D and zoom in to max zoom, its getting destroyed.
this fix is might be relative to 3D Mode.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

3 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @emackey!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@ggetz
Copy link
Contributor

ggetz commented Jun 12, 2023

@cesium-concierge stop

@ggetz
Copy link
Contributor

ggetz commented Feb 21, 2024

I'm closing this for now as it's quite out of date with main. If we wish to revisit this in the future, let's open a fresh PR. Thanks all.

@ggetz ggetz closed this Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Notable backlog items
Development

Successfully merging this pull request may close these issues.

Polyline with arrow material is blurry
6 participants