-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Adds Mars ellipsoid and trackable rovers #12828
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @mzschwartz5! ✅ We can confirm we have a CLA on file for you. |
@mzschwartz5 please share the sandcastle via https://sandcastle.cesium.com/ so reviewers without a local JS setup can view it. |
^This happened in the meantime. More generally: Once there is that ✔️ for the build process, the sandcastle on a branch with the name (I also tried to remember adding this link in the first post when applicable) |
Potential for automation via GitHub Actions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great so far!
I love that we're using models for both the rovers. Maybe consider using minimumPixelSize so that they are visible further away?
@@ -254,6 +254,8 @@ Ellipsoid.MOON = Object.freeze( | |||
), | |||
); | |||
|
|||
Ellipsoid.MARS = Object.freeze(new Ellipsoid(3396190.0, 3396190.0, 3371726.0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder before this PR is merged: This property needs inline documentation, and the addition should be documented in CHANGES.md
.
Currently using a minimum pixel size of 64 (defined in the edit: I think it's actually the |
@mzschwartz5 here are my notes from our call:
|
@LisaBosCesium Should all be addressed now (pushing changes momentarily). On the point of adjusting lighting, here are some before and after screen shots: |
Nice!
Requests:
|
Re Jezero/Perseverance - Understood, but just as we label both Gale and Curiosity, we should label Jezero as well as Perseverance because some people will be looking for Jezero. Re instructions - I'm so used to those default instructions they are literally invisible to me - didn't notice you'd added to them. OK, let's just keep this as is.
The behavior felt inconsistent when I was testing, but I don't think it's worth messing with more unless you've already made a change that feels like an improvement to you. |
😂 I totally get it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the code seems fine. This is definitely a very lengthy sandcastle but I understand it's one that's more oriented at a "sales" perspective so that's ok.
Had a number of comments just using the sandcastle itself. Some are definitely optional, others I feel not so but feel free to push back on if they've already been discussed. Largely just ideas/suggestions
- Marco mentioned this but at certain angles the camera "bounces" off the terrain much higher than I would expect.
- (optional) Can we make the camera/animation tips only show up the first time? I don't need to see them every time I switch rovers
- The highlight animation is cut off by the element bounds
- [image]
- The clock keeps playing after leaving a rover, should it?
- Also why not just start the clock for people when zooming to a rover? we're already resetting the time to the start
- Shadows don't turn off correctly when switching the rover "mid-flight"
- Counter point, why even enable shadows at all? Can we just turn them off completely?
- Repro:
- Run sandcastle
- Click Play on animation
- Pick Curiosity
- Pick Perseverance before getting to Curiosity
- See shadows
- "Spin on first load" is not working. This is because you're calling
reset
in the first menu item. That handler will always be called by default when first setting it up - The "Landing site" POIs overlap with the nearby crater names. Can we remove them? Or make them only appear much closer? Or maybe combine them like "Gale crater (Perseverance)"?
- The order of POIs may not be the "best"? I realize they're alphabetical but I was first exploring them "top to bottom" and certain collections don't make sense (optional if it's already been decided)
- For example There's a POI between "Curiosity Landing Site" and "Gale crater". Going "top to bottom" makes the camera jump back and forth to the same spot even though it's right next to it. Same idea around the Perseverance
- It'd be nice to follow the POIs along the Martian Journey "in order"
- (optional) It might help a lot to use
optgroup
elements to group things like "Craters" or "Rovers" or "Martian Journey stops" - Should all the positions be in a single menu list? (easier if using optgroups)
- With Menus you can't "re-select" the current selection. if you select a rover to see the path, then go to a POI, you can't jump back to the same rover. If they were one list it'd be easier to do that.
- The menu doesn't reset or change once leaving a POI. I don't think it needs to but it's a bit weird to zoom to a POI but still see it listing the rover you were just at
- Did we look into keeping the camera facing somewhat down when using
flyTo
? It's a little odd to just stare at stars for some flights. If not possible then it's fine - (optional) is there any way to nicely cut out sections of the rover's path? It's not very interesting to see it just shake in one spot for may Sols because it didn't move much.
- Curiosity barely moves until Sol 300. Can we just start it further in their path?
- Perseverance also has a very long section it's "stationary" around the 250-300 sol mark. Honestly "max" speed of
604800x
or whatever seems better than the current100_000
Apps/Sandcastle/gallery/Mars.jpg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please shrink this waaaay down to match the other thumbnails. Should be ~250px in width
@@ -254,6 +254,14 @@ Ellipsoid.MOON = Object.freeze( | |||
), | |||
); | |||
|
|||
/** | |||
* An Ellipsoid instance initialized to a sphere with the mean radii of Mars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we say where we sourced our value from? For the earth WGS84 is well known and clear. Just trying to get ahead of someone asking
Apps/Sandcastle/gallery/Mars.html
Outdated
scene.camera.flyTo(landmark); | ||
}; | ||
|
||
const toolbarMenuEntries = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're keeping 2 lists this should be renamed to make it clear in the code it's a different list than the first time this was declared.
Yes... definitely pulled as much as I could out into data, but it gets lengthier every time I revise upon feedback :)
I think some of the other changes may be more trouble than they're worth / lengthen the sandcastle more (e.g. cutting out sections of rover data, first-time-only camera tips, optgroups and other menu shenanigans) or have already been discussed in other peoples' feedback (e.g. we do want the POIs in alphabetical order). But on the whole, many of these are easy adjustments that will improve the experience. |
Fixes spin-on-load, camera pitch while flying, label overlap, rover animation UX, and disables shadows.
Description
Sandcastle demo for Mars terrain. Showcases geological points of interest, and the ability to see a few rovers take their journeys from landing point to current point - defined in a CZML file, data and models from NASA.
Issue number and link
N/A
Testing plan
Make sure to include the CesiumGS default Ion access token when testing.
Author checklist
CONTRIBUTORS.md
CHANGES.md
with a short summary of my change