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

Render only on changes #470

Merged
merged 10 commits into from
May 16, 2016
Merged

Conversation

squirrelo
Copy link

This moves rendering to an event-based action, where new frames are only rendered when the views are changed in some way. previously it was rendering a new frame regardless of it was needed or not.

@coveralls
Copy link

coveralls commented May 12, 2016

Coverage Status

Coverage remained the same at 93.381% when pulling 47b91f6 on squirrelo:render-only-on-change into 81aca23 on biocore:new-api.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.381% when pulling 39aaa2b on squirrelo:render-only-on-change into 81aca23 on biocore:new-api.

@squirrelo
Copy link
Author

No more cooking computer or really loud fan noise!

@coveralls
Copy link

coveralls commented May 12, 2016

Coverage Status

Coverage remained the same at 93.381% when pulling e092280 on squirrelo:render-only-on-change into 81aca23 on biocore:new-api.

@@ -209,6 +209,9 @@ define([
controller.resize(w, h);
}
});

// Set for update
this.sceneViews[0].needsUpdate = true;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be a loop over all the sceneViews?

Copy link
Author

Choose a reason for hiding this comment

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

If any sceneview needs updating all are, so I just used 0 since it will always exist.

Copy link
Member

Choose a reason for hiding this comment

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

Is that always true?

Maybe I missed something but on the design that we did a while ago each scene view was independent so I'm unsure why if 1 needs to change all of them need to.

Copy link
Member

Choose a reason for hiding this comment

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

@josenavas, yes you are right, this made me realize that in a situation with more than one ScenePlotView3D object, we are updating all the objects if one of them needs update, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the logic on line 223 works exactly like I said above.

@ElDeveloper
Copy link
Member

👍 code looks good! And functionality is great! CPU usage went from 105 % to 3-5% in a 10,000 samples plot.

@coveralls
Copy link

coveralls commented May 13, 2016

Coverage Status

Coverage remained the same at 93.381% when pulling 4b36bff on squirrelo:render-only-on-change into 81aca23 on biocore:new-api.

@ElDeveloper
Copy link
Member

Looks good, all this needs is tests and it should be good to go.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.381% when pulling 9c0172f on squirrelo:render-only-on-change into 81aca23 on biocore:new-api.

@coveralls
Copy link

coveralls commented May 13, 2016

Coverage Status

Coverage remained the same at 93.381% when pulling a348d99 on squirrelo:render-only-on-change into 81aca23 on biocore:new-api.

@squirrelo
Copy link
Author

Comments addressed, ready to go.

}
var scope = this;
$.each(this.sceneViews, function(i, sv) {
console.log(i, sv);
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this line?

@coveralls
Copy link

coveralls commented May 16, 2016

Coverage Status

Coverage decreased (-1.3%) to 92.063% when pulling 2e83eca on squirrelo:render-only-on-change into 81aca23 on biocore:new-api.

@ElDeveloper ElDeveloper merged commit 83c486e into biocore:new-api May 16, 2016
@ElDeveloper
Copy link
Member

🎆

@colinbrislawn
Copy link

I'm so glad this is a thing now. I'm looking forward to the next stable release of Emperor.

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.

5 participants