-
Notifications
You must be signed in to change notification settings - Fork 47
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
Allow bigger eye separation #498
Conversation
I don't agree with that. Unlimited ranges for naturally limited values highlight some other issues here. The default value assumes this to be in meters, so most likely the model scale where this value is used for is wrong. The limits of values a almost useless now if we allow for unlimited values, also UI components can no longer have meaningful sliders. |
@@ -379,7 +379,7 @@ void OSPRayEngine::_createCameras() | |||
PropertyMap::Property eyeSeparation{"interpupillaryDistance", | |||
"Eye separation", | |||
0.0635f, | |||
{0.f, 10.f}}; | |||
{0.f, 1e31f}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before doing these kind of changes to params that are dynamically rendered by the UI, please test the UI as well! As @tribal-tec mentioned, this will make the sliders and input controls for this property unusable.
I advise reverting this commit and finding a different solution to whatever problem this was trying to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to remove the limits here, so no slider component should be used then if I'm not mistaken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, any property that is {'type': "number"'}
will have a slider. But regardless of that, the input will still jump a high amount when using the keyboard arrows or mouse wheel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires careful thinking I'm afraid. You may have a similar problem with simulation timestamps and simulation playback speed for very long simulations. I don't remember seeing any integrated UI component that allows choosing a number with high precision when the limits are very large... The only solution that I can think of is a bit hacky and complexifies the UI. The idea would be to detect that the range is very large (unless a hint is provided from json) and split the slider in two, one for the significand and other for the order of magnitude. Another solution is to have a normal slider that goes from 0 to 1000 and a combo box for the multiplier (u, m, 1, K, M, ...)
If this is physically based then 10 is as incorrect as infinite since it should be a constant. What is needed then is a way to either globally scale the world or scale the size of the human. Is there any parameter available today to do that scaling? |
For as long as we do not know what to do there, we need to remove the limit for the movie sequences that we have to generate. So please do not rollback this change until we have an acceptable solution. |
No description provided.