Skip to content

Layer ordering #285

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

Merged
merged 9 commits into from
May 25, 2018
Merged

Layer ordering #285

merged 9 commits into from
May 25, 2018

Conversation

kchadha
Copy link
Contributor

@kchadha kchadha commented May 16, 2018

Resolves

scratchfoundation/scratch-vm#1066

Proposed Changes

Create layer groups to organize the many different kinds of layers and ordering needs for those layers on the stage.

Layer groups can be either explicitly or implicitly ordered. Explicitly ordered layer groups are strictly ordered based on a provided ordering preference. Layer groups that are explicitly ordered cannot have the items within their group re-ordered (e.g. via setDrawableOrder). Layer groups that are implicitly ordered are initially ordered based on what order the drawables are created (each new one getting added in front of the previous ones in that group). Items in implicitly ordered groups (e.g. sprites) can get re-ordered via setDrawableOrder.

Layer groups are themselves ordered. An initial set of layer groups (and their ordering) is determined via the new setLayerGroupOrdering function. If a drawable is created in a group that is not already known, the group gets created and put in front of all known groups. The ordering type (explicit vs. implicit) of the not previously known is determined based on whether an explicit ordering preference is provided for the drawable being provided (e.g. if explicit ordering is provided, create the new group as an explicitly ordered group, otherwise the new group is an implicitly ordered group).

createDrawable, destroyDrawable and setDrawableOrder each take information about what group the drawable belongs to. If a group is not provided, it will be added to a special internal 'unspecified layer group' which has implicit ordering. Items added to this group will appear in front of all other items (including any new groups that were created as described above).

Reason for Changes

scratchfoundation/scratch-vm#1066

Test Coverage

Existing tests pass (as long as VM also gets updated). Tried to add new unit tests, but the tests are failing because of a dependency of scratch-svg-renderer depending on window and self (so leaving new tests as TODO #287).

Related PR

scratchfoundation/scratch-vm#1148 <-- should be merged before this one.

@towerofnix
Copy link
Contributor

Handy! Just out of curiosity's sake, will this layer group system be usable by extensions in general?

@kchadha
Copy link
Contributor Author

kchadha commented May 16, 2018

@towerofnix It can be! It's designed to accommodate whatever arbitrary layer groups and ordering a scratch-render user wants, but right now it's being used with the corresponding changes in scratchfoundation/scratch-vm#1148, which defines an 'extension' layer which just contains pen and video for now.

Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

In general this looks good, but I'm curious about performance (see comment). If that's fine then consider this PR approved :)

if (this._unspecifiedLayerGroup) { // Add unspecified items at the very end
this._drawList.concat(this._unspecifiedLayerGroup.drawables);
}
}

This comment was marked as abuse.

This comment was marked as abuse.

@kchadha
Copy link
Contributor Author

kchadha commented May 21, 2018

@cwillisf, this PR is ready for re-review.

I've done away with explicitly ordered layer groups and am making the group argument required in all the functions it was added to. Both of these changes make the code much cleaner and easier to reason about. We can replace the explicitly ordered layer group functionality by just having some more groups (e.g. see scratch-vm PR: pen and video have been broken out into separate layer groups).

@kchadha kchadha requested a review from cwillisf May 21, 2018 13:40
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, and I don't see anything that stands out to me as a performance concern. Have you found that it performs better than the old code?

@kchadha kchadha merged commit 7f53a4a into scratchfoundation:develop May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants