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

Replace get/set functions with properties #1421

Closed
pjcozzi opened this issue Jan 30, 2014 · 6 comments
Closed

Replace get/set functions with properties #1421

pjcozzi opened this issue Jan 30, 2014 · 6 comments

Comments

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 30, 2014

Should be done throughout the API for consistency and to help stabilize it now that property access is fast.

For example:

var centralBody = widget.centralBody;
var ellipsoid = centralBody.getEllipsoid();
var scene = widget.scene;
var primitives = scene.getPrimitives();
var camera = scene.getCamera();

will become

var centralBody = widget.centralBody;
var ellipsoid = centralBody.ellipsoid;
var scene = widget.scene;
var primitives = scene.primitives;
var camera = scene.camera;
@mramato
Copy link
Member

mramato commented Jan 30, 2014

👍 As long as we are just replacing functions, we shouldn't notice a speed difference (on Chrome). Replacing public variables with properties (i.e. Cartesian3.x) would definitely still have an affect performance.I would advise whomever works on this to start by changing a single class (i.e. Scene) and opening a PR for that. We'll probably want to streamline the documentation to make it easy to tell if something is readonly or not. We normally do this by starting the doc with either just Gets or Gets or sets.

@shunter
Copy link
Contributor

shunter commented Jan 30, 2014

Firefox performance is comparable to functions when the properties are defined on the prototype (which is what we typically do anyway for memory reasons when replacing get/set functions). Properties on instances are still slow. FF bug: https://bugzilla.mozilla.org/show_bug.cgi?id=626021

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Jan 30, 2014

All sounds good. No intention to change any public variables.

@hpinkos
Copy link
Contributor

hpinkos commented Mar 25, 2014

@pjcozzi I think I've covered all of the major ones, which other files did you want changed over?

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Mar 25, 2014

Let's update the following types. They have trivial get functions that take no arguments that can become @readonly get properties. The few that also have set functions are marked and should also be updated.

Since this will be a large change, it can be split into a few pull requests.

Core:

  • EllipsoidGeodesic
  • Event
  • Fullscreen
  • LeapSecond (get/set)
  • Occluder

Renderer (no need to add doc where it is DOC_TBA since most of this will be @Private but preserve existing doc)

  • AutomaticUniforms (getSize and getDatatype, not getValue)
  • Buffer (get/set)
  • Context (get/set)
  • CubeMap (get/set)
  • CubeMapFace
  • Framebuffer
  • Renderbuffer
  • ShaderProgram
  • Texture (get/set)
  • TextureAtlas
  • UniformState (get/set) (this will be a fair amount of work so it could be its own pull request)
  • VertexArray

Scene:

  • AnimationCollection
  • CentralBodySurface (get/set)
  • Credit
  • Tile
  • TerrainData and derived types
  • Appearance and derived types

DynamicScene will be updated separately so don't worry about files in that folder. Also, Timeline and JulianDate will have bigger refactors so don't worry about them.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Mar 26, 2014

@hpinkos I changed the above list to checkboxes to make this a bit easier to track.

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

4 participants