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

Changed viewport to floats in order to increase browser compatibility #438

Merged
merged 1 commit into from
Jun 22, 2018

Conversation

favreau
Copy link
Member

@favreau favreau commented Jun 22, 2018

Some browsers process viewport size as floats rather than uint. Brayns now exposes floats through the ws API but still uses unsigned int internally.

@favreau favreau requested a review from rolandjitsu June 22, 2018 13:11
@rdumusc
Copy link

rdumusc commented Jun 22, 2018

Hmm from my external point of view this change looks wrong. Viewports can't have non-interger sizes and I can't see any reason to do that.

@favreau
Copy link
Member Author

favreau commented Jun 22, 2018

They actually can, according to most browsers, hence the change. We did search the web and found out that the API used to get sizes of components in web pages are floating point values. Can't remember the API that we looked at, but @rolandjitsu knows them.

@favreau favreau closed this Jun 22, 2018
@favreau favreau reopened this Jun 22, 2018
@rolandjitsu
Copy link
Contributor

rolandjitsu commented Jun 22, 2018

@rdumusc sending values from the client side will send doubles, depending on the OS, browser, etc.

See getBoundingClientRect() and DOMRect, which is what I use to get the current width/height of the user's viewport.

So it might make sense to accept such values.

You could argue that I can round the values, but what if other clients use our API and do not know of this issue and cannot understand what is happening?

@rdumusc
Copy link

rdumusc commented Jun 22, 2018

No it doesn't, unless I'm missing a part of the picture here. No matter what the JS API is giving you Brayns will never use floating point value. So I can't see what is the benefit of exposing meaningless values...

@hernando
Copy link
Contributor

hernando commented Jun 22, 2018

I don't know what the rational behind the DOMRect properties is, but they look inspired in a vector graphics world to me. The undebatable truth is that the framebuffer is pixel based. I don't fully understand the diff to be honest, but if your intention is to announce the viewport size as float values because that's what browsers like, it's OK. But allowing the client to set a non integer viewport size in Brayns looks definitely wrong. The client should deal with that because it's who knows how to map a discrete size image into whatever the browser uses.

@rdumusc
Copy link

rdumusc commented Jun 22, 2018

annoncing the viewport size as float is also wrong ;-)

@favreau
Copy link
Member Author

favreau commented Jun 22, 2018

Do we seriously have to debate on that???

@rolandjitsu
Copy link
Contributor

@rdumusc and @hernando while I understand your concerns (or not), accepting a float instead of an int does not have any impact on anything. It only removes the need of code duplication from clients (where each client would need to do the rounding). And most of the clients will be browsers, so why not accept the same type that the browser will be sending?

@rdumusc
Copy link

rdumusc commented Jun 22, 2018

No we don't have to debate, I have nothing to gain here since it's not my project. Do whatever you want... ;-)

@rolandjitsu
Copy link
Contributor

@rdumusc this also addresses an issue we have currently which I fixed on my end, but cannot deploy without breaking production.

@hernando
Copy link
Contributor

I was just wondering whether this can bite you later on because I don't see where's the rounding (is it really rounding, or flooring?) occurring now.

@rolandjitsu
Copy link
Contributor

@favreau do you just cast to int or do you round anywhere?

@favreau
Copy link
Member Author

favreau commented Jun 22, 2018

The default c++ cast is used. Only the integer value remains.

@favreau favreau merged commit c4fc48e into BlueBrain:master Jun 22, 2018
@tribal-tec tribal-tec added the bug label Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants