-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaKernel] Clean API message for Image loading #503
[SofaKernel] Clean API message for Image loading #503
Conversation
return FactoryImage::CreateObject(loader, filename); | ||
extension = std::string(filename, p+1); | ||
Image* createdImage = FactoryImage::CreateObject(extension, filename); | ||
if( extension.compare("default")>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.
Could you explain me the reason for using string::compare
please?
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.
@fredroy why not using:
if( extension != "default" )
[ci-build] |
{ | ||
helper::vector<std::string> validExtensions; | ||
helper::io::Image::FactoryImage::getInstance()->uniqueKeys(std::back_inserter(validExtensions)); | ||
msg_error("Image") << "No extension detected. Valid extensions: " << validExtensions; |
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.
Tips of the day:
If you have a lot of msg_error("xxx") you can use the macro
MSG_REGISTER_CLASS(....)
After using it you can just do msg_info() <<
search example of how to use it in the sofa source code.
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.
Is there doc on this macro @damienmarchal ?
I wrote the macro outside the namespace in the .h file but it did not work. Apparently there is some tricks but I don't want to make any forward definition or write the macro in the .cpp
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.
There is no tricks, the macro is just a plain gold old macro that replace the macro with a longer text. I don't understand the connection you make with forward definition and not having the macro used in the .cpp. But you can find a lot of uses of the macro in the code base (eg: SphereLoader.cpp, meshTrian.cpp, ...)
I definitively support anyone writing the corresponding docs in the messaging documentation :)
} | ||
} | ||
else | ||
{ | ||
if ((m_positions.getValue()).size() == 0 && (m_vertices2.getValue()).size() == 0) | ||
{ | ||
sout << "VisualModel: will use Topology." << sendl; | ||
msg_info() << "VisualModel: will use Topology."; |
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.
No need to prefix "VisualModel:" as it the emitter is part of the msg_info in a normalized way.
@@ -1515,16 +1514,16 @@ void VisualModelImpl::handleTopologyChange() | |||
|
|||
if(!is_in_shell) | |||
{ | |||
sout << "INFO_print : Vis - triangle is forgotten in SHELL !!! global indices (point, triangle) = ( " << last << " , " << ind_forgotten << " )" << sendl; | |||
msg_info() << "INFO_print : Vis - triangle is forgotten in SHELL !!! global indices (point, triangle) = ( " << last << " , " << ind_forgotten << " )"; |
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 here...remove "INFO_print:"
msg_info() << "INFO_print : Vis - lastIndexVec[i] = " << lastIndexVec[i]; | ||
msg_info() << "INFO_print : Vis - tab.size() = " << tab.size() << " , tab = " << tab; | ||
msg_info() << "INFO_print : Vis - t_local rectified = " << triangles[j_loc]; | ||
msg_info() << "INFO_print : Vis - t_global = " << t_forgotten; |
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.
The proper way for this kind of message is:
msg_info() << "Vis - last = " << last << msgendl
<< "Vis - lastIndexVec[i] = " << lastIndexVec[i] << msgendl
<< Vis - tab.size() = " << tab.size() << " , tab = " << msgendl
....
Doing this, emit only one message which allow correct message visualization and reporting in the GUI (eg in the message windows of GUI runSofa2). It is also more efficient as the cost of message processing is divided by 5.
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.
thanks for the feedback, I'll take it into account
Thank Mr tablot for cleaning message. I like this small but yet usefull PR. |
hey @damienmarchal |
Hard to tell without seeing what you are doing but at the top of .cpp it should be after the includes and before the namespaces (otherwise the namespace of the define get embeded). |
I confirm the macro is indeed after the includes and before the namespaces |
The requirement for merged are met. What about passing to ready and merging ? |
[ci-build] |
Minor commit to improve messaging when image cannot be loaded.
Before : no error, no message, no hints for available extensions to use
This PR:
Reviewers will merge only if all these checks are true.