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

RenderAll() changes #2545

Merged
merged 2 commits into from
Oct 12, 2015
Merged

RenderAll() changes #2545

merged 2 commits into from
Oct 12, 2015

Conversation

asturur
Copy link
Member

@asturur asturur commented Oct 12, 2015

Current rendering stack and possible improvements.

First I will describe the current renderAll situation.
We start with a _renderBackground.
renderBackgrond calls _draw(backgroundImage).
_draw function has this scheme:
save context
apply viewportTransform
check if the object should be rendered and actually render it
(this means object !== activeGroup || this.preserveObjectStacking)
restore context
renderObjectControls ( exit if not !this.active or render the controls )
_renderObjects ( rendering of all objects )
render all the objects in stack, or exclude the one in activeGroup depending on preserveObjectStacking and checking if active group contains them
Every object is drawn with _draw, so again save context, apply viewportTransform, again check if the object is in activeGroup,
restore context, renderControls.
_renderActiveGroup (rendering objects of activeGroup) with the
_draw procedure already described.
_renderOverlay mirrored from _renderBackgroundImage
_renderControls if controlsAboveOverlay, make a global call for rendering controls.

  • What '_draw' does? wax on and wax off the viewport transform for every object, wrong in my opinion. and check if an object should be rendered... But all object should be rendered now or later.
  • What 'shouldRenderObject' does? basically obey to preserveObjectStacking that decide if we draw all the objects together or if we render before the normal objects and later the activeGroup.
    Because objects in activeGroup are still present on canvas, in the objects array, so we need to be careful to do not draw them twice.

All the checks are about being in the activeGroup, so useless for activeGroup itself, useless for background and overlay images.

Proposed changes in this PR.
background image and overlay image are rendered with their render method.
ViewportTransform get applied once for renderAll, with one save and restore context.

Objects are divided into two groups immediately, and then we render the groups one after the other without additional checks.

Because of this changes take over _draw functionality, the _draw function disappear.

All the controls in the canvas are drawn with canvas.drawcontrols.

What actually changes in the rendering stack? controls are always on top of the object pile, (not above overaly ) regardless if the object is on top.
But consider that we have this bug as of now, and this new situation would solve it.
The bug is that when controls are drawn during the stack, they are present on canvas when the objects after the 'active index' are rendered, so any eventual global composite operataion get influenced by them

So in my opinion moving the control in the top of the stack is a good move, also code reduction is noticiable and we should see minor performance improvements.

Example of bug:
image

@@ -868,10 +864,17 @@
if (this.clipTo) {
fabric.util.clipContext(this, canvasToDrawOn);
}

objsToRender = this._chooseObjectsToRender();
Copy link
Member Author

Choose a reason for hiding this comment

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

Following your advice i grouped the big block of code and gave it a clear ( i hope ) name.

kangax added a commit that referenced this pull request Oct 12, 2015
@kangax kangax merged commit e322764 into fabricjs:master Oct 12, 2015
@asturur asturur deleted the ne branch October 13, 2015 00:13
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.

2 participants