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

Globe Projection (PR2) #4640

Merged
merged 6 commits into from
Jun 7, 2020
Merged

Globe Projection (PR2) #4640

merged 6 commits into from
Jun 7, 2020

Conversation

Pessimistress
Copy link
Collaborator

For #4638

Change List

  • Add project_get_orientation_matrix shader function
  • Docs

@Pessimistress Pessimistress changed the title project shader module supports globe projection Globe Projection (PR2) Jun 3, 2020
@coveralls
Copy link

coveralls commented Jun 3, 2020

Coverage Status

Coverage decreased (-0.01%) to 80.405% when pulling 5de5eee on x/globe-02 into 15bb681 on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Nice, this looks like a fairly limited change. You have already done a good job making sure there are docs and tests, though I think adding a "Usage" section to the doc or a "project module developer's guide" would probably be a good idea. Just a bit more context on the use case to make it easy to understand why this function is there and how it should be used would be good.

In the background I also think it might be worth at start thinking about introducing WGS84 coordinates and just about how these project module functions would look. I am hoping we can take that opportunity to "conceptually generalize" this module.

@@ -53,6 +53,8 @@ const float TILE_SIZE = 512.0;
const float PI = 3.1415926536;
const float WORLD_SCALE = TILE_SIZE / (PI * 2.0);
const vec3 ZERO_64_LOW = vec3(0.0);
const float EARTH_RADIUS = 6370972.0;
const float GLOBE_RADIUS = 256.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comments about units etc. Especially globe radius is not automatically clear. Why do we want globes to be in 256 units, some relation to flat WebMercator probably and maybe some choices we make in common space?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1 common unit maps to 1 screen pixel at zoom=0. It is indeed arbitrary but offers roughly the same "zoom level" expectation as the web mercator projection.

@@ -123,6 +138,14 @@ vec4 project_position(vec4 position, vec3 position64Low) {
);
}
}
if (project_uProjectionMode == PROJECTION_MODE_GLOBE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: These nested if statements are getting pretty big, may read cleaner if we have helper functions projectGlobe projectFlat etc.

modules/core/src/shaderlib/project/project.glsl.js Outdated Show resolved Hide resolved
docs/shader-modules/project.md Show resolved Hide resolved
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.

None yet

3 participants