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

Add View helpers #2174

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Add View helpers #2174

merged 2 commits into from
Oct 27, 2023

Conversation

jailln
Copy link
Contributor

@jailln jailln commented Aug 24, 2023

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.

examples/laz_dragndrop.html Outdated Show resolved Hide resolved
src/Core/Prefab/GlobeView.js Outdated Show resolved Hide resolved
@jailln jailln force-pushed the feat/map-3d-extent branch from b5a903c to f2c81c6 Compare August 29, 2023 13:37
@jailln jailln requested a review from ftoromanoff September 11, 2023 08:44
@ftoromanoff
Copy link
Contributor

ftoromanoff commented Sep 13, 2023

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...

Copy link
Contributor

@ftoromanoff ftoromanoff left a comment

Choose a reason for hiding this comment

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

I get the point that private variable in JS might not be well handled.

For the second point: using methode getVariable() or using setter/getter I can't say what is the best usage, we might need to discuss that internaly.

@jailln
Copy link
Contributor Author

jailln commented Sep 13, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

jailln commented Oct 10, 2023

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 jailln force-pushed the feat/map-3d-extent branch from f2c81c6 to 428173b Compare October 11, 2023 15:54
@jailln
Copy link
Contributor Author

jailln commented Oct 11, 2023

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.

@jailln jailln requested a review from ftoromanoff October 11, 2023 15:56
@jailln jailln self-assigned this Oct 11, 2023
@jailln jailln force-pushed the feat/map-3d-extent branch from 428173b to 2786ab1 Compare October 11, 2023 16:02
Use `View.renderer` to get the threejs renderer of the view
Use `View.camera3D` to get the threejs camera of the view
@jailln jailln force-pushed the feat/map-3d-extent branch from 2786ab1 to f346c46 Compare October 13, 2023 09:57
@jailln jailln merged commit ef8d3f4 into iTowns:master Oct 27, 2023
7 checks passed
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