search

pro3d-space / PRo3D

PRo3D, short for Planetary Robotics 3D Viewer, is an interactive 3D visualization tool allowing planetary scientists to work with high-resolution 3D reconstructions of planetary surfaces.
http://pro3d.space/
GNU Affero General Public License v3.0
38 stars 2 forks source link

Bugs/snapshot frustum #358 #370

Closed RebeccaNowak closed 5 months ago

RebeccaNowak commented 7 months ago

Bugfix #358 Frustum.withAspect from Aardvark.Media has unexpected behaviour. Replaced calls to the function. Now updating Frustum in FrustumModel to be consistent with Model.frustum. Also setting formerly unused Model.aspect on resize of rendering control.

haraldsteinlechner commented 5 months ago

for the record, what is the reason for storing aspect in the model?

RebeccaNowak commented 5 months ago

I suspect that is has historical reasons, that we save the frustum in Model AND FrustumModel. Both are used in the code in different places, but I think we could remove Frustum from Model and only store it in FrustumModel. I could open a new Issue with the task to look into this and clean it up, if you don't see a problem with removing Frustum from Model.

haraldsteinlechner commented 5 months ago

Hm, cool thanks for explaining. Taking it further, conceptually, the model should be "viewer" independent (so when e.g. a browser also opens a connection there is no race for updating the frustum). So best would be to have no width/height/aspect dependent stuff in the model at all. Since this seems unrealistic (is it?), your approach in removing redundancy would be a good option. I'm not sure if it pays out though the effort (given the fact that having aspect dependent stuff in the model is not preferred anyways).

So let's go with this PR and let's create an issue which also talks about the possibility to really remove the aspect as a whole as a long-term objective.

What do you think?

RebeccaNowak commented 5 months ago

Sounds good to me! Not sure yet what the implications of completely removing the aspect would be, but we could definitely look into this.