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

Optional fixed draw order #1888

Merged
merged 3 commits into from Mar 27, 2014
Merged

Optional fixed draw order #1888

merged 3 commits into from Mar 27, 2014

Conversation

twpayne
Copy link
Contributor

@twpayne twpayne commented Mar 21, 2014

The fact that the draw order of features with the same z index is undefined has caused some confusion on the mailing list.

This PR adds an optional fixedOrder property to ol.layer.Vector that specifies that features should be drawn in a fixed order. At the moment this fixedOrder is taken from the feature's goog.getUid, which is a proxy for the order in which the features were created. Note that this is not the same order as the features might have been added to the source, but will often correspond to the order in which they were deserialised.

You can see the effect of the fixed order in the KML example by trying the following commands in the console:

map.getLayers().getAt(1).setFixedOrder(true);
map.getLayers().getAt(1).setFixedOrder(false);

The disadvantage of using a fixed order this that extra memory and time is required to sort the features before rendering.

There's probably a better name for the property than fixedOrder.

@twpayne
Copy link
Contributor Author

twpayne commented Mar 25, 2014

Any comments on this? Travis does pass, it errored because GitHub was being DDOSed at the time.

@twpayne
Copy link
Contributor Author

twpayne commented Mar 27, 2014

I've now updated this according to the discussion in the hangout. This makes ol3 render in a consistent order by default, while still adding the flexibility to use an undefined render order (to avoid the sort if you have many features) and even a custom render order (if you want to use some other property).

You can see the effect of changing draw order by running the kml example and typing the following into the console:

vector.setRenderOrder(null); // set undefined rendering order
vector.setRenderOrder(undefined); // set default rendering order
vector.setRenderOrder(function(f1, f2) { return Math.random() - 0.5; }) // random!

@tschaub
Copy link
Member

tschaub commented Mar 27, 2014

Looks good to me.

At some point, we might want to get consistent with our use of goog.array.sort v. Array.prototype.sort (we rely on the latter being what we expect when finding points in rings, while modifying features, and in using libtess.js.

twpayne added a commit that referenced this pull request Mar 27, 2014
@twpayne twpayne merged commit ddac5c5 into openlayers:master Mar 27, 2014
@twpayne twpayne deleted the draw-order branch March 27, 2014 18:50
// the order in which they were created. This also helps to ensure that
// object properties are always added in the same order, which helps many
// JavaScript engines generate faster code.
goog.getUid(this);
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for doing that in ObjectEvent instead of Object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch :) Total error on my part! PR coming up.

goog.array.sort(features, vectorLayerRenderOrder);
goog.array.forEach(features, renderFeature, this);
} else {
vectorSource.forEachFeatureInExtent(extent, renderFeature, this);
Copy link
Member

Choose a reason for hiding this comment

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

I think we will never end up here because vectorLayerRenderOrder is set if undefined (line 186)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're right. I think the check at line 237 should be !goog.isNull(...).

Copy link
Member

Choose a reason for hiding this comment

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

PR to come

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.

None yet

4 participants