[Enhancement] Fluid setters

CesiumJS
1

Dear all:

What about implemeting fluid setters for most used objects ?

This has been discussed in PR 383[1].

The idea would be to simplify some code:

var polyline = this.polylines_[event.trackIndex],

    color = polyline.getColor();

polyline.setWidth(2);

color.alpha = 1;

polyline.setColor(color);

could become

this.polylines_[event.trackIndex].setWidth(2).getColor().setAlpha(1); // also requires the addition of setAlpha on Color

Any feedback on this proposal ?

[1] https://github.com/AnalyticalGraphicsInc/cesium/pull/383#issuecomment-11389840

2

HI Victor,

Thanks for the ideas.

I think fluid setters is a positive change, but I expect many get/set functions to be deprecated in time. Our trend is to use less get/set functions and use properties instead. Once browsers get their act together and have fast get/set functions for properties, I expect that we’ll use these everywhere instead of plain get/set functions. Generally, we only use set functions when we need to do something extra when a value is changed, e.g., set a dirty flag. Anyway, I’d like to hear some more opinions, but I am OK with this change with the caveat that these functions are likely to go away in the future.

As for the setAlpha function, that would not work without adding notifications because the Polyline needs to know when any part of the color changes, which is why I originally suggested a setColorAlpha function on the Polyline itself. However, I’m not sure that I still recommend this because we plan to add materials to Polylines, which will deprecate the color get/set. I expect this to happen in the next few months so I’d rather not introduce a function that we already know will be removed.

Regards,

Patrick

3

Thanks for the insights.

          Have you already thought about using such things

as MVCObject[1].

    Tom Payne               has

an Apache2 implementation[2].

    Events can be used             to do

something extra (ie setting a dirty flag ) when
properties are updated.

    [1] [https://developers.google.com/maps/documentation/javascript/reference#MVCObject](https://developers.google.com/maps/documentation/javascript/reference#MVCObject)

    [2] [https://github.com/twpayne/mvcobject](https://github.com/twpayne/mvcobject)
4

I lean heavily towards not making this change, but I’m open to having a discussion about it to hear what other people’s thoughts are. As Patrick alluded to, ultimately we want to get rid of our getters/setters, not add to them. That alone might be a good enough reason not to spend time changing them. Here are my other concerns.

  1. I’m under the impression that functions that return a value are much slower in all browsers than functions that don’t return a value. This could be outdated information, but we should at least benchmark it. No matter what, replacing simple get/set properties with functions will at least slow things down a little and I’m not sure the performance trade-off is worth it.

  2. I find code like you suggest makes everything less readable. In Cesium, we strive for clean, readable code over clever code.

  3. We strive to be consistent, making this change for Color would mean all objects should behave that way (such as Cartesians). That’s a pretty invasive change without a lot of payoff.

I’ll continue to think about this, and I’d like to hear any counter-arguments.

Matt

5

At the widget/application level, we definitely want to move towards a MVC/MVVM architecture with such notifications. The goal is that our UI is completely data driven by the underlying model layer and the widgets themselves are just layout and wiring. However, at the engine level, I’m concerned it would add an incredible amount of overhead and make everything way to slow.

6

Matt,

  I'll a          lso continue to think about

this.

      I currently               only have a few

days of experience with your code base . I
definitely need to get som e more
experience.

              Thanks for your promp                      t

replies.

7

The MVCObject code ended up in OpenLayers 3 as the ol.Object class. In
OpenLayers 3 we only use it for high level objects (maps, layers,
collections of layers, markers) and not for low level objects like
colors and coordinates. It's very much there because I found MVCObject
in the Google Maps API to be very powerful, particularly when it came
to binding object properties together (e.g. ensuring that a HTML popup
follows a marker as it is dragged around).

We wrestled with a number of design constraints related to this. Some
observations/constraints that we had:
- OpenLayers 3 has to run on a variety of older browsers, so we
couldn't use newer or non-standard JavaScript language features.
- We wanted the ability to be able to fire events at the instant a
property is changed, rather than detecting changes at a later time.
- We wanted to use the Closure Compiler in advanced mode with full
type checking, which adds further constraints.

Fluent interfaces are popular in some fashionable JavaScript
libraries, but they have many disadvantages for large projects.

As a particularly problematic example, consider the use of combined
getters and setters in a fluent interface (I know Victor wasn't
suggesting this, but this is what Leaflet looks like):

  camera.position([5, 45, 1000]).lookAt([5.5, 45.5, 0]); // set
camera's position and lookAt
  var p = camera.position(); // returns the camera position

This causes several problems:

1. The position() function must check its arguments every time to work
out whether it is being called as a setter or a getter; this is a
performance price that is paid every function call.

2. Arguments must be converted into their implicit types, e.g. the
array [5, 45, 1000] must be converted into a Cartesian3 (or similar);
once again this performance price must be paid every time.

3. In fluent interfaces, there is often a lot of flexibility in the
types accepted by functions. This tends to prevent the virtual machine
from optimizing the code.

4. The return type of position() varies: in setter mode it returns a
camera ("this"), in getter mode it returns a Cartesian3. This makes
type checking hard.

For information, in OpenLayers 3, we use separate getters and setters
for all public properties. This provides a consistent API with the
ability to hook in extra functionality if needed. We rely on the
Closure Compiler to inline these getters and setters as direct
property accessors when there is no extra functionality to avoid the
performance penalty of the extra function call.

So, overall, I think fluent interfaces are perfectly fine in small
projects without performance concerns. However, they do cause problems
for larger projects or when performance matters.

If you haven't already seen it, this talk explains clearly how to
write code that V8 will optimize well:
  http://v8-io12.appspot.com/

Regards,
Tom

8

Tom,

To be clear, what you say is that fluent à la Leaflet is bad ! (I think we all agree here).

However “return this;” in setters should not be a pb whatever the size of the project if you use such an optimizer as Google Closure.

Did I get you right ?

(Thanks for the v8 link)

9

I think Cesium is somewhat unusual compared to most other JavaScript libraries in that raw low-level performance becomes extremely important when trying to keep a scene rendering at 60fps while doing so much heavy calculation. As a result, we often have to make tradeoffs to keep performance high over other concerns, whereas many other lightweight JavaScript projects often seem to treat performance as an afterthought.

So, even though V8 has done amazing work in making JavaScript as fast as it now is, it still works best when we write the stupidest code possible, even though often that can be harder to read. As a result, we tend to make a habit of writing things like typeof x === ‘undefined’ everywhere (faster than coercion to bool), using simple for (i = 0, len = etc; i < len; ++i) style loops (much faster than Array.forEach), avoiding variadic functions that use “arguments”, using the “result” out parameter pattern to avoid allocations by reusing instances, favoring raw properties over methods, and get/set methods over ES5 properties, and many more. With something as large as Cesium, tiny inefficiencies in critical places can actually cause significant performance problems that can be hard to find using a profiler.

10

(The following applies to Google Closure Compiler code, sorry for this
off-topic relative to Cesium)

Having setters return "this" causes problems with the Google Closure
Compiler because of type checking issues related to inheritance.

Consider a base class that has a setter setX that returns this. The
type of the return value is the base class:

  var Base = function() {};
  Base.prototype.setX = function(x) {
    this.x = x;
    return this;
  };

Now derive a child class from this, and add a setY method to the child:

  var Child = function() {};
  goog.inherits(Child, Base);

  Child.prototype.setY = function(y) {
    this.y = y;
    return this;
  }

So far, so good, except that we can't use a fluent interface:

  var c = new Child();
  c.setX(1).setY(2); // will not compile

The reason is that the return type of setX is Base, and a Base has no
setY method.

At the moment, as far as I know, there is no way of telling the
compiler that the return type of setX is the same type as the
receiver.

Regards,
Tom

11

Ok Tom, makes sense. Thanks.

    (and sorry for triggering
      this off-topic)