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

Fallback to gl.canvas.height when gl.canvas.clientHeight are not avai… #2421

Merged
merged 1 commit into from
Dec 12, 2018

Conversation

sakitam-fdd
Copy link
Contributor

For #2296

Background

Change List

Fallback to gl.canvas.height when gl.canvas.clientHeight are not available

@CLAassistant
Copy link

CLAassistant commented Nov 10, 2018

CLA assistant check
All committers have signed the CLA.

@1chandu
Copy link
Contributor

1chandu commented Nov 12, 2018

This was already fixed here :#2405 , please open an issue with details if you need do make any changes. Closing.

@1chandu 1chandu closed this Nov 12, 2018
@sakitam-fdd
Copy link
Contributor Author

@1chandu Yes, I am very happy to see your changes. But I don't think this change is fully functional. In my last test, I found that in addition to the deck.js needs to be rolled back, the draw-layer also needs to be rolled back. So I made this change.

@fuzhenn
Copy link

fuzhenn commented Dec 12, 2018

Hi @1chandu ,

#2405 is not enough to fix the off-screen canvas bug described in #2296. getGLViewport will still blank the canvas as it only reads canvas.clientHeight as viewport height which is always 0 when canvas is off-screen. It also needs to fallback to canvas.height when canvas.clientHeight is 0.

As I have tested, this PR will have getGLViewport get the correct viewport height and perfectly fix the off-screen bug, please consider to reopen this PR and have it merged.

Thanks very much!

@fuzhenn
Copy link

fuzhenn commented Dec 12, 2018

cc @Pessimistress

@Pessimistress Pessimistress reopened this Dec 12, 2018
@1chandu 1chandu merged commit 4004719 into visgl:master Dec 12, 2018
@1chandu
Copy link
Contributor

1chandu commented Dec 12, 2018

@fuzhenn , this looks good, #2405 missed getGLViewport, I just merged into master.

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

5 participants