-
Notifications
You must be signed in to change notification settings - Fork 46
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 'default-bvh-flag' command line option #599
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.
A test would be nice :) Showing that less memory is actually used.
|
||
enum class BVHType | ||
{ | ||
dynamic, |
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.
that's very ospray centric. what is 'none' even? Shouldn't it be a bitmask also? dynamic + compact sounds reasonable to me. If ospray can't do that, we shouldn't limit it here. 'Static' is missing also to me.
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.
Since optimizations for a particular metric typically go against others, I think it's fair to have a single label to provide a hint about the expected usage.
engines/ospray/OSPRayModel.cpp
Outdated
@@ -510,6 +510,29 @@ bool OSPRayModel::_commitTransferFunction() | |||
return true; | |||
} | |||
|
|||
void OSPRayModel::_commitBVHType() |
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.
ospCommit(_model)
is missing then, otherwise the function should be renamed.
How do I query the memory usage of the BVH? |
I think checking the ram usage is a bit hard so I added a simpler unit test. |
brayns/common/utils/Utils.h
Outdated
} | ||
|
||
template <typename T> | ||
inline bool enumHas(const T eA, const T eB) |
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.
template<typename T>
bool isBitSet(typename std::underlying_type<T>::type mask, const T bit)
brayns/common/utils/Utils.h
Outdated
@@ -34,4 +36,18 @@ inline auto lowerCase(std::string str) | |||
std::transform(str.begin(), str.end(), str.begin(), ::tolower); | |||
return str; | |||
} | |||
|
|||
template <typename T> | |||
inline T enumSet(const T eA, const T eB) |
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.
inline T enumSet(const T eA, const T eB) | |
template <typename T> | |
inline typename std::underlying_type<T>::type setBit(const std::underlying_type<T>::type mask, const T bit) | |
{ | |
return mask | static_cast<typename std::underlying_type<T>::type>(bit); | |
} |
You should return the underlying type, because semantically the output value is not the enum anymore.
But even worse, imagine you have this code
enum class A : int { a = 1, b = 2 };
int f(const A a)
{
switch(a)
{
case A::a: return int(a);
case A::b: return int(a);
}
}
That code if perfect, but if you call f with something A(17), the function return value will be undefined.
// | ||
(PARAM_DEFAULT_BVH_FLAG.c_str(), | ||
po::value<std::vector<std::string>>()->multitoken(), | ||
"Default bvh type [static|dynamic|compact|robust]"); |
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.
It's not clear that you can have multiple flags and what is the token separator.
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.
BVH
@@ -234,6 +234,7 @@ class GeometryParameters : public AbstractParameters | |||
return _morphologyUseSDFGeometries; | |||
} | |||
|
|||
BVHType getDefaultBVHType() const { return _defaultBVHType; } |
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.
int getDefaultBVHModes or getDefaultBVHTypeMask
engines/ospray/OSPRayModel.cpp
Outdated
|
||
ospSet1i(_model, "dynamicMode", dynamicMode ? 1 : 0); | ||
ospSet1i(_model, "compactMode", compactMode ? 1 : 0); | ||
ospSet1i(_model, "robustMode", robustMode ? 1 : 0); |
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.
ospSet1i(_model, "robustMode", robustMode ? 1 : 0); | |
ospSet1i(_model, "robustMode", isBitSet(_bvhType, BVHTtype::dynamic)); |
brayns/common/scene/Model.h
Outdated
@@ -97,9 +97,12 @@ class ModelParams : public ModelInstance | |||
const std::string& getName() const { return _name; } | |||
void setPath(const std::string& path) { _updateValue(_path, path); } | |||
const std::string& getPath() const { return _path; } | |||
void setBVHType(BVHType bvhType) { _updateValue(_bvhType, bvhType); } | |||
BVHType getBVHType() const { return _bvhType; } |
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.
It would be better if this parameter goes into the ModelDescriptor property map because then you can set it from the client. I'm not totally sure about the jsonrpc part, maybe @rolandjitsu can comment on this.
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.
If you add it to the property map it would be rendered without any effort on my side (because it's generated from schema). Otherwise I'd need to add it myself.
But then you have to make sure that when the bvh property on the property map is set, you do your stuff (set the bvh type).
Also, I'm not sure how enums are handled currently for the property map props (if they are).
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.
In fact it's not an enum, it's a bit mask. From the command line I think it's set as a space separated list of strings. I'd guess you don't have any code for rendering such a thing.
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, we don't handle bit masks.
For multiple selection we'd need some additional info from schema (instead of {type: 'string', enum: ['bla', ...]}
has to be {type: 'array', items: {type: 'string', enum: ['bla', ...]}
).
And then I'd need to add support for multiple selection.
If this is single selection, then we just need string enums instead of bitmask.
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.
@tribal-tec requested it to be a multiple selection, but not all combinations are allowed... Another option is splitting this into multiple properties:
- BVH update hint: static|dynamic
- compact BVH: bool
- robust BVH: bool
What do you think @karjonas ?
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 should be easy to render and we only need to extend the schema to support single selection for a set of values.
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.
As discussed offline, let's keep it as it is for the moment and open this can of worms later.
d36fbd5
to
e12851b
Compare
brayns/common/scene/Model.h
Outdated
{ | ||
_bvhFlags = std::move(bvhFlags); | ||
} | ||
std::set<BVHFlag> getBVHFlags() const { return _bvhFlags; } |
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.
const std::set<BVHFlag>&
(PARAM_DEFAULT_BVH_FLAG.c_str(), | ||
po::value<std::vector<std::string>>()->multitoken(), | ||
"Sets one or more property flags to be used by default when creating " | ||
"a bvh [dynamic|compact|robust]"); |
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.
BVH
Same as before, it's not clear that this is a list? Or does boost program_options take care of indicating it?
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 don't know how to explain it without a wall of text.
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.
"Set a default flag to apply to BVH creation, one of [dynamic|compact|robust], may appear multiple times."
I'm not sure whether --default-bvh-flag "dynamic robust" is also accepted though.
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.
It is.
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.
Then, the message is not totally accurate, but at least it doesn't miss the possibility of setting multiple flags.
if (kv != BVH_TYPES.end()) | ||
_defaultBVHFlags.insert(kv->second); | ||
else | ||
throw std::runtime_error("Invalid bvh type '" + bvh + "'."); |
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.
type -> flag
engines/ospray/OSPRayModel.cpp
Outdated
const bool compactMode = _bvhFlags.count(BVHFlag::compact); | ||
const bool robustMode = _bvhFlags.count(BVHFlag::robust); | ||
|
||
ospSet1i(_model, "dynamicMode", dynamicMode ? 1 : 0); |
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.
ospSet1i(_model, "dynamicMode", dynamicMode ? 1 : 0); | |
ospSet1i(_model, "dynamicMode", _bvhFlags.count(BVHFlag::dynamic)); |
const char* app = testSuite.argv[0]; | ||
const char* argv[] = { | ||
app, "demo", "--default-bvh-flag", "robust", "--default-bvh-flag", | ||
"compact", |
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 have never guessed this is how you provide multiple flags through the command line...
No description provided.