-
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
Add simulation data support in OptiX engine #737
Conversation
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.
Can you add a test as well?
ae4f547
to
4fc66fb
Compare
Issues fixed |
brayns/engine/Model.cpp
Outdated
@@ -595,4 +602,60 @@ AbstractSimulationHandlerPtr Model::getSimulationHandler() const | |||
{ | |||
return _simulationHandler; | |||
} | |||
|
|||
bool Model::commitSimulation() |
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.
Isn't the transfer function used for volumes (i.e. not only for simulation)?
brayns/engine/Model.cpp
Outdated
|
||
bool Model::_commitTransferFunction() | ||
{ | ||
if (!_transferFunction.isModified() && _hasCommitedTransferFunction()) |
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.
I would remove _hasCommitedTransferFunction. I know you're just translating the original code, but the performance benefit is debatable while it complicates maintenance (the purpose of the function is not clear and forces any coder to look at the derive classes implementation to get the full picture).
brayns/engine/Model.cpp
Outdated
|
||
const auto animationFrame = _animationParameters.getFrame(); | ||
|
||
if (_hasCommitedSimulationData() && |
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.
Same comment as for the transfer function.
I even think it's completely unnecessary because once you have called _commitSimulationDataImpl, this part of the condition is always true and the first time you evaluate it, the second part is always false because the current animation frame gets updated in line 652 (and only there).
|
||
modelDesc->getModel().getSimulationHandler()->waitReady(); | ||
// NOTE: Replace for-loop with samples per pixel when available in OptiX | ||
for (int i = 0; i < 100; ++i) |
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.
Do you need to many samples to make it consistent?
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.
Good job!
No description provided.