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

Incorrect distance field of view calculation for negative elevation #1655

Closed
HarelM opened this issue Sep 17, 2022 · 13 comments · Fixed by #2858
Closed

Incorrect distance field of view calculation for negative elevation #1655

HarelM opened this issue Sep 17, 2022 · 13 comments · Fixed by #2858
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed Terrain 3D

Comments

@HarelM
Copy link
Collaborator

HarelM commented Sep 17, 2022

This is in continue to the following PR which removed the elevation offset from terrain 3D: #1578

The situation can be see in the following diagram:
IMG-2568

And can be reproduced in the local environment here when running npm run start-debug
http://localhost:9966/test/debug-pages/terrain-satellite.html#13.45/31.54164/35.33225/49.6/54

More info in the following comment:
#1578 (comment)

@prozessor13
Copy link
Collaborator

I did not analyzed in detail, but for documentation here is an example where the far-clipping plane is not correct:

http://localhost:9966/test/debug-pages/terrain-satellite.html#10.23/43.0154/9.1969/-97

@HarelM
Copy link
Collaborator Author

HarelM commented Sep 30, 2022

There's also this strange artifact around there, not sure it is related to this issue but might be a problem with the DEM:
image
http://localhost:9966/test/debug-pages/terrain-satellite.html#11.53/42.7216/9.247/173.4/73

@prozessor13
Copy link
Collaborator

I think the spikes are a data-problem.

And i think the far-clipping-plane error is because the dem-tiles have the bathymetry included (e.g. contains negative-coordinates, like the dea-sea).

@HarelM
Copy link
Collaborator Author

HarelM commented Mar 3, 2023

Assigning bounty M.
Link to parent Bounty: maplibre/maplibre#189

@HarelM HarelM added the 💰 bounty M Medium Bounty, USD 250 label Mar 3, 2023
@typebrook
Copy link
Contributor

I did not analyzed in detail, but for documentation here is an example where the far-clipping plane is not correct:

http://localhost:9966/test/debug-pages/terrain-satellite.html#10.23/43.0154/9.1969/-97

Attach image for this case:
Screenshot_2023-05-26-10-29-25_1920x1080

@HarelM
Copy link
Collaborator Author

HarelM commented Jul 11, 2023

I think I manages to think about a solution while looking at the code.
There's a method of calculating the minimum DEM for a tile.
If we store that along side the elevation in the terrain object we can use that when calculating the FOV.
I'm not sure the code is correct, but it seems to behaving nicely in general.
I'll open a draft PR later today so that other can see the horrible code I wrote :-)
cc: @maxammann.

@HarelM
Copy link
Collaborator Author

HarelM commented Jul 11, 2023

I've opened a PR for feedback: #2858
cc: @acalcutt, can you check if this fixes the issue you've seen with the bathymetry stuff?

@birkskyum
Copy link
Member

Where is the reproduction found now? The link suggested http://localhost:9966/test/debug-pages/terrain-satellite.html#13.45/31.54164/35.33225/49.6/54 doesn't appear to be working anymore

@HarelM
Copy link
Collaborator Author

HarelM commented Jul 16, 2023

I've took the page from an old version of the repo, added it, fixed the bug and removed the page as I'm not sure we are allowed to use that data in the examples.
You can take a look at the commits in my PR.

@maxammann
Copy link
Contributor

If this is not urgent, then I can review the changes. Though right now there is not much time I can invest.

@HarelM
Copy link
Collaborator Author

HarelM commented Jul 16, 2023

I was looking for your input since you said you were looking in this code.
I generally would like to push this bug fix through...

@acalcutt
Copy link
Contributor

I will also try to take a look at this soon. I just got back from a week of vacation.

@HarelM HarelM removed the 💰 bounty M Medium Bounty, USD 250 label Jul 19, 2023
@HarelM
Copy link
Collaborator Author

HarelM commented Jul 19, 2023

Removed bounty as this is mostly solved by the linked PR.
Any chance for a review on it? Would be great to have it merged and released...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed Terrain 3D
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants