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

Don't render if width or height is 0. #896

Merged
merged 4 commits into from
Jun 27, 2013
Merged

Don't render if width or height is 0. #896

merged 4 commits into from
Jun 27, 2013

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jun 25, 2013

Fixes #834

I'm not sure if there's a better way to do this at a level lower than the widget, but it seem like a problem that needs to be addressed as part of the render loop.

@shunter
Copy link
Contributor

shunter commented Jun 25, 2013

I still get an immediate crash when setting the canvas to display: none.

@mramato
Copy link
Contributor Author

mramato commented Jun 25, 2013

What exactly are you doing? I've tried several different things and it works fine for me.

@shunter
Copy link
Contributor

shunter commented Jun 25, 2013

As briefly discussed offline, I discovered that Scene is calling clientHeight and clientWidth on the canvas every frame, so there seems to be no reason why we shouldn't just call CesiumWidget.resize every frame. I'm still investigating, but so far I've removed the "every 60 frame" check from both CesiumWidget and Viewer, resizing every frame, with no apparent performance impact.

@mramato
Copy link
Contributor Author

mramato commented Jun 25, 2013

Sounds good to me. I don't know why we didn't think of that earlier.

@shunter
Copy link
Contributor

shunter commented Jun 25, 2013

I tried to do a bit of archaeology to figure out when we stopped resizing every frame. I couldn't find it, but I know for Santa I reduced how often resize was called to lower CPU usage. That may have been primarily to help performance for 2D in IE, or possibly browsers have improved since then.

I tested Chrome and FF, and don't see any CPU difference nor profiler difference between resizing every frame and the current behavior. I'll commit to this branch and you can check my work.

Removes the need for window resize listeners as well.
@mramato
Copy link
Contributor Author

mramato commented Jun 26, 2013

So can't we just remove resize and add the code to render? I'm not sure I see the use case for these two things being in different functions anymore.

@mramato
Copy link
Contributor Author

mramato commented Jun 27, 2013

@shunter I'd like to get this in to b18, are there any hold-ups?

@shunter
Copy link
Contributor

shunter commented Jun 27, 2013

Having resize and render be separate functions allows for the possibility for an external render loop that doesn't resize every frame, in case the widget is in an app where it is known to be a fixed size.

@mramato
Copy link
Contributor Author

mramato commented Jun 27, 2013

Fair enough I guess. So is this ready?

@shunter
Copy link
Contributor

shunter commented Jun 27, 2013

I think so, yes.

@shunter
Copy link
Contributor

shunter commented Jun 27, 2013

I was going to see if @emackey could try this in our internal Cesium app that has a complicated HTML UI to make sure there's no problems with the resize.

@emackey
Copy link
Contributor

emackey commented Jun 27, 2013

Doesn't appear to add any additional hurt to the performance.

@mramato
Copy link
Contributor Author

mramato commented Jun 27, 2013

As we saw, @emackey's app had no noticeable performance degradation. I updated CHANGES. Merging. Thanks @shunter

mramato added a commit that referenced this pull request Jun 27, 2013
Don't render if width or height is 0.
@mramato mramato merged commit 77e54d7 into master Jun 27, 2013
@mramato mramato deleted the issue834 branch June 27, 2013 18:24
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.

Hiding map widget causes error
3 participants