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

Merge projection view module #9

Merged
merged 11 commits into from
Sep 24, 2014
Merged

Conversation

tallytalwar
Copy link
Member

  • view module contains an instance of projection now.
  • needed for further support for multiple map projection support in future.


ViewModule::ViewModule(float _width, float _height) {
ViewModule::ViewModule(float _width, float _height, ProjectionType _projType) {
setMapProjection(_projType);
Copy link
Member

Choose a reason for hiding this comment

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

Where possible, I think we should consolidate functionality such that code is minimally repeated. For instance, add a projection argument to init() and call setMapProjection within init(). Needing to update code in multiple locations leads to a lot of problems in my experience.

@bcamper
Copy link
Member

bcamper commented Sep 22, 2014

cc @iwillig - check this out and add any comments!

@bcamper
Copy link
Member

bcamper commented Sep 22, 2014

@rmarianski, you might also have some feedback on this.

@@ -109,7 +109,7 @@ class MercProjection : public MapProjection {
* _type: type of map projection, example ProjectionType::Mercator
* _tileSize: size of the map tile, default is 256
*/
MercProjection(ProjectionType _type, int _tileSize=256);
MercProjection(int _tileSize=256);
Copy link
Member Author

Choose a reason for hiding this comment

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

Left tileSize in there for the time being.. if we are not using any mercator pixel conversion method then we can get rid of this.

Copy link
Member

Choose a reason for hiding this comment

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

You need some unit to measure tiles so you can do things like calculate how many tiles wide/high the viewport should be for a given zoom level in standard web mercator view. It doesn't have to be "pixels", but it's as good as anything else (and the long-time standard, so I think it's clearer than using something else). With retina/high-density displaces, it's more an abstract unit now than an actual pixel, but that's a problem across all app/web development these days anyway, so not unique here...

@rmarianski
Copy link
Contributor

I read through the diff and nothing big jumped out at me. I just had a couple of comments here and there about some pieces.

👍

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.

4 participants