Skip to content

Fix various typings#1132

Closed
xawill wants to merge 4 commits intoNASA-AMMOS:masterfrom
xawill:fix/types-misc
Closed

Fix various typings#1132
xawill wants to merge 4 commits intoNASA-AMMOS:masterfrom
xawill:fix/types-misc

Conversation

@xawill
Copy link
Copy Markdown
Contributor

@xawill xawill commented May 13, 2025

I am not sure exactly what is public API, but I think at least the typings for Ellipsoid were just missing.

For the cameras of TilesRenderer, I am not sure if this is supposed to be exposed. My use case was to compute the camera elevation in the intersectsTile function of a custom BaseRegion from LoadRegionPlugin. Let me know if you see a better way to do it.

@gkjohnson gkjohnson added this to the v0.4.10 milestone May 15, 2025
export class TilesRenderer<TEventMap extends TilesRendererEventMap = TilesRendererEventMap> extends TilesRendererBase implements EventDispatcher<TEventMap> {

ellipsoid: Ellipsoid;
cameras: Camera[];
Copy link
Copy Markdown
Contributor

@gkjohnson gkjohnson May 20, 2025

Choose a reason for hiding this comment

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

"cameras" isn't intended to be part of the public API and as such it's not documented. Do you need this field? Can you explain how you're using it?

edit

I just reread your main comment:

My use case was to compute the camera elevation in the intersectsTile function of a custom BaseRegion from LoadRegionPlugin

Can you explain this a bit more? You're reusing the cameras in a custom region plugin? What exactly is the intent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not exactly. I am using the standard LoadRegionPlugin but with a custom region. In intersectsTile, I would like to have the camera elevation to only activate the region if the camera is close enough to the ground.
To be concrete, the custom region is the country of Switzerland for which is available a more detailed tileset than Google's and I would like Google's entire globe for visual consistency at high level view and once we zoom closer to Switzerland have another dataset for which I use LoadRegionPlugin to load.
See example code here.

Comment on lines +40 to +42
calculateHorizonDistance( latitude: number, elevation: number ): number;
calculateEffectiveRadius( latitude: number ): number;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These functions were added for GlobeControls and I don't want to consider them public at the moment. If they're going to be made public I'd like to consider some more descriptive names for the functions.

Copy link
Copy Markdown
Contributor Author

@xawill xawill Aug 9, 2025

Choose a reason for hiding this comment

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

I am using calculateEffectiveRadius in trigonometry calculations (linear distance between two GPS coordinates) for an animation of globe rotation to know if the place the camera is currently looking at is within a certain tolerance radius of the animation destination. If it is in the radius, the camera doesn't move; if it is outside, the camera animates.
See example here

@gkjohnson gkjohnson modified the milestones: v0.4.10, v0.4.11 May 30, 2025
@gkjohnson gkjohnson modified the milestones: v0.4.11, v0.4.12 Jun 29, 2025
@gkjohnson gkjohnson modified the milestones: v0.4.12, v0.4.13 Jul 12, 2025
@gkjohnson
Copy link
Copy Markdown
Contributor

Closing for now

@gkjohnson gkjohnson closed this Jul 15, 2025
@gkjohnson gkjohnson removed this from the v0.4.13 milestone Jul 15, 2025
@xawill
Copy link
Copy Markdown
Contributor Author

xawill commented Aug 9, 2025

Thank you @gkjohnson for taking a look and sorry for not getting back to you earlier.

I saw some of the changes were already fixed in #1174, but there is an error there I am now fixing.

I also answered your comments. I am now rebasing this PR; would you mind reopening it? Thx!

@xawill
Copy link
Copy Markdown
Contributor Author

xawill commented Sep 16, 2025

@gkjohnson In case you missed my comment above, any chance reopening the PR please?

@gkjohnson
Copy link
Copy Markdown
Contributor

Sorry I missed this previously - it looks like I'm not able to reopen the PR (see the grayed out button below). Can you make a new one?

image

@xawill xawill mentioned this pull request Sep 18, 2025
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.

2 participants