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

Hillshade layer improvements #378

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

farfromrefug
Copy link
Contributor

I create this first as a draft because changes still need to happen.
Here what is in this PR:

  • added options
    • illuminationDirection
    • illuminationMapRotationEnabled
  • changed default hillshade parameters for a better default render
  • added ElevationDecoder and more explicitly MapBox and Terrarium decoders
  • decoders are used for hillshade rendering and elevation computation
  • getElevation and getElevations from HillshadeRasterTileLayer, MapBoxElevationDataDecoder and MapBoxElevationDataDecoder (decoders require a datasource too in the arguments)
  • added possibility to link those decoders with Valhalla proxy to query elevation.

Now here are some remarks

  • getElevation and getElevations right now transform poses from wgs84. How to handle that correctly? Does it mean a projection argument need to be added?
  • valhalla wont actually use the decoders to compute route elevation costing. I thought it would but i discovered Valhalla tiles need to be computed with elevation (or more precisely grade) for it to work. What i did will only allow Valhalla to use our decoder for the "height" route. Which is in fact not a real need anymore as you can do the same directly from the layer or the decoder
  • i am not sure about where mutex shoud be used or not. Especially in getElevation and getElevations

PS: i did manage to get elevation costing to work with Carto/Valhalla. It is really easy and could be added if you want in your packages. It even have no consequences on the package size. Let me know if you need any help in implementing that.

@farfromrefug
Copy link
Contributor Author

@mtehver removed the valhalla changes as they dont seem necessary for now.

The rest is now fully working

@farfromrefug farfromrefug marked this pull request as ready for review August 27, 2020 12:35
@farfromrefug
Copy link
Contributor Author

@mtehver any news on this? I would really like it to get released

@mtehver
Copy link
Contributor

mtehver commented Sep 23, 2020

@farfromrefug Thanks for reminding, I will have time to review the changes on Friday.

@mtehver
Copy link
Contributor

mtehver commented Sep 25, 2020

A few high-level issues that should be resolved before merging:

  • Elevation data type should be double, not int. It should be be basically the 'z component' of MapPos and use the same measurement unit as layer projection (currently both EPSG3857 and EPSG4326 use meters)

  • I do not understand the logic in illumination direction calculation. First, is there a need for 2 illumination modes? In general the lighting should change if the map is rotated. For special cases it is possible to use MapRendererListener and reconfigure lighting direction based on view parameters, thus it can be done already. The lighting direction in HillshadeRasterTileLayer in PR is currently defined as a single floating point parameter, which is not enough to define direction in 3D space. Also, it does not work in globe mode, as the direction vector should be based on local tangent frame.

  • I would remove the dependency on data sources from ElevationDecoder class, thus would remove getElevation, getElevations and getMapTileBitmap methods from the class. A better place for the code would be HillshadeRasterTileLayer class. The idea behind this is to make call graphs and dependencies between modules simpler and place the code that deals with coordinate transformations and projections into layers.

@farfromrefug
Copy link
Contributor Author

Just about the illumination. Sometimes you dont want the illumination to change with rotation. When you are out in the mountain looking at "shapes" you want to rotate the map to your orientation but still use the shadows to see slope shapes.
I actually always disable illumination with rotation ;)
Will look at your comments, thanks

@farfromrefug
Copy link
Contributor Author

farfromrefug commented Oct 27, 2020

@mtehver finally found the time to work on it. But i will need your help with the light orientation issue.
If i use _mainLightDir i dont have the expected effect as the light comes from "under the ground" as the z value is negative.
It creates a wrong illumination where all the side of a mountain are in the shadow. Setting the z to positive value fixes it in planar but not spherical.
also i am not sure how to respect mainLightDir and still allow to use normalIlluminationMapRotationEnabled and normalIlluminationDirection
I am wondering if we should ignore mainLightDir for hillshading.
Fixing the rest

To show it in context. Here is what is wanted
Screenshot_1603826287
And here is what i have with mainLightDir
Screenshot_1603826292

@mtehver
Copy link
Contributor

mtehver commented Oct 28, 2020

About _mainLightDir, the lighting vector should be in world space, not in tangent space. The following snippet will transform tangent space vector to world space vector:

_mainLightDir = cglib::vec3<float>::convert(cglib::unit(viewState.getProjectionSurface()->calculateVector(internalFocusPos, options->getMainLightDirection())));

Note that you need a reference point, which can be 'focus point' in internal coordinates.

I think in 'normalIlluminationMapRotationEnabled' mode the light vector should be rotated along the focus point normal axis. This is something I can implement myself, if you want, once the other changes are ready.

@farfromrefug
Copy link
Contributor Author

@mtehver yes i think i would feel better with you implementing it. I am lost there:s I pushed the requested changes

Dont use readBitmapColor with floats as sometimes the average can give wrong rounded value and thus wrong elevation.
This fixes -5000 elevations
@farfromrefug
Copy link
Contributor Author

@mtehver found an issue with my getElevation algorithm in some edge case.
In my app my HillshadeTileLayer is now using an ordered data source combining 2 mbtiles:

  • world mbtiles for zooms 5-6.
  • france mbtiles for zooms 5-11

The issue comes from the fact that getElevation tries to get the tile bitmap using dataSource->getMaxZoom() but it wont work outside of France because 11 has no tile in those regions.
My question is this, is there a way to find the lowest zoom tile at a MapPos?
If so maybe i could try to access that tile if getMapTileBitmap for maxZoom fails. That would allow be to not make it slower for most cases but work in my edge case.

@mtehver
Copy link
Contributor

mtehver commented Feb 1, 2021

@farfromrefug If I understand correctly, the question is about 'missing tiles'? Note that datasource can notify layer about such tiles with 'replace with parent' flag in TileData. It seems that the PR does not handle this case, the flag is simply ignored in getMapTileBitmap.

@farfromrefug
Copy link
Contributor Author

@mtehver yes that s it ! Now i see how to fix that. But then TileUtils::CalculateMapTileBounds(mapTile, projection) will be wrong because it is for a lower level. I need to find a way to "update" mapTile so that i reflects the tile we actually used to get the bitmap.
Will look in the repo if you do something like that anywhere else

@farfromrefug
Copy link
Contributor Author

@mtehver I updated that PR to develop and got the getElevation and getElevations to work correctly with isReplaceWithParent.

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