search

iTowns / itowns

A Three.js-based framework written in Javascript/WebGL for visualizing 3D geospatial data
http://www.itowns-project.org
Other
1.09k stars 293 forks source link

Add View helpers #2174

Closed jailln closed 10 months ago

jailln commented 1 year ago

Add some helper functions in View to get threejs renderer, renderer size and camera to avoid having to know itowns internal structure to access them.

ftoromanoff commented 12 months ago

By adding helper functions to get specific variable, should we not modify these variables to set them as private ? We can also instead of implementing function to get variables, use setter and getter ? I don't really know what is the best way to work around these from a architectural/api point of view...

jailln commented 12 months ago

I think setting them as private could be done but it is not related to this PR. In addition, I'm not fond of private fields implementation in JS (side effects, badly interpreted by dev tools). But this is another subject :)

Regarding getter vs the methods I implemented, in my opinion both are valid and it's only a matter of coding style and API. I don't have a strong opinion on which one we should used, as long as it's decided quickly. Please let me know, so we can move on quickly.

ftoromanoff commented 11 months ago

I think setting them as private could be done but it is not related to this PR. In addition, I'm not fond of private fields implementation in JS (side effects, badly interpreted by dev tools). But this is another subject :)

Regarding getter vs the methods I implemented, in my opinion both are valid and it's only a matter of coding style and API. I don't have a strong opinion on which one we should used, as long as it's decided quickly. Please let me know, so we can move on quickly.

After discussion, I think we should opt for the setter/getter methods. In the case of camera being already in use (by the itowns.camera) we might use get camera3D or any other name and add a deprecation warning to get camera (wich should be internaly rename _camera.

jailln commented 11 months ago

Sorry I don't understand what you mean by:

In the case of camera being already in use (by the itowns.camera) we might use get camera3D or any other name and add a deprecation warning to get camera (wich should be internaly rename _camera.

Can you give more details please ?

jailln commented 11 months ago

I changed to implemented to getters, removed the getRendererSize because one can use View.renderer and then gets the size if one needs it. I also added a commit to document View properties (including camera3D and renderer).

Let me know if that's what you meant by your last comment.