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

Less API indirection? #2183

Closed
pjcozzi opened this issue Oct 7, 2014 · 2 comments
Closed

Less API indirection? #2183

pjcozzi opened this issue Oct 7, 2014 · 2 comments

Comments

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 7, 2014

If we just created viewer, we need to go quite deep to access things:

viewer.scene.camera./*...*/;
viewer.scene.primitives.add(/*...*/);
viewer.scene.imageryLayers.add(/* ... */);

viewer represents the container and UI, and scene represents the 3D scene - this is exactly what we want inside Cesium, but externally, there is a lot of indirection.

How crazy is it to combine scene and viewer into one object perhaps with some JavaScript magic so that viewer gets all the scene properties. Perhaps we would even rename viewer to scene.

var scene = new Cesium.Scene('cesiumContainer');
scene.camera./*...*/;
scene.primitives.add(/*...*/);
scene.imageryLayers.addImageryProvider(/* ... */);

I'm not married to the idea, just asking.

CC #2114

@mramato
Copy link
Contributor

mramato commented Oct 7, 2014

I think the better approach would be to just expose ES5 properties on Viewer that call through to the underlying scene. We could only do this for the most common properties to keep the api clean. We actually were doing this at one point but someone asked they be removed to avoid having the property in two places. (I think it was you 😛)

I'm not sure combining them magically is the correct way to go. If we really wanted to do that, then I would make scene an implementation detail and only expose selected properties through Viewer. I'm also not sure how big of a deal this is since when I wrote code I would just do add var scene = viewer.scene and then user scene directly throughout.

Technically CesiumWidget has this problem too, so we would have to decide if we want similar changes there or we could just treat it as "lower-level" and have it expose scene like it does now.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Oct 8, 2014

I think the better approach would be to just expose ES5 properties on Viewer that call through to the underlying scene. We could only do this for the most common properties to keep the api clean

I think this would be fine for Viewer and CesiumWidget.

I think it was you

I would not be surprised.

I would just do add var scene = viewer.scene and then user scene directly throughout.

I agree it is not a problem for non-novice users, but our API requires touching too many objects to do simple things compared to similar APIs so if we can do very trivial things to improve this, I think it is a win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants