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

Jump to bottom

[SofaKernel] Clean API message for Image loading #503

Merged
merged 2 commits into from Dec 5, 2017
Merged

[SofaKernel] Clean API message for Image loading #503

merged 2 commits into from Dec 5, 2017

Conversation

hugtalbot
Copy link
Contributor

@hugtalbot hugtalbot commented Oct 30, 2017 •
edited by damienmarchal

Minor commit to improve messaging when image cannot be loaded.
Before : no error, no message, no hints for available extensions to use

This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@hugtalbot hugtalbot added enhancement About a possible enhancement pr: clean Cleaning the code labels Oct 30, 2017
guparan
guparan reviewed Oct 31, 2017
View reviewed changes
SofaKernel/framework/sofa/helper/io/Image.cpp Outdated
return FactoryImage::CreateObject(loader, filename);
extension = std::string(filename, p+1);
Image* createdImage = FactoryImage::CreateObject(extension, filename);
if( extension.compare("default")>0 )
Copy link
Contributor

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?

Copy link
Contributor Author

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" )

@guparan guparan added the pr: status to review To notify reviewers to review this pull-request label Oct 31, 2017
@guparan
Copy link
Contributor

guparan commented Nov 6, 2017

[ci-build]

@hugtalbot hugtalbot self-assigned this Nov 8, 2017
@hugtalbot hugtalbot added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Nov 8, 2017
damienmarchal
damienmarchal reviewed Nov 8, 2017
View reviewed changes
SofaKernel/framework/sofa/helper/io/Image.cpp
{
helper::vector<std::string> validExtensions;
helper::io::Image::FactoryImage::getInstance()->uniqueKeys(std::back_inserter(validExtensions));
msg_error("Image") << "No extension detected. Valid extensions: " << validExtensions;
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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 :)

damienmarchal
damienmarchal reviewed Nov 8, 2017
View reviewed changes
SofaKernel/modules/SofaBaseVisual/VisualModelImpl.cpp Outdated
}
}
else
{
if ((m_positions.getValue()).size() == 0 && (m_vertices2.getValue()).size() == 0)
{
sout << "VisualModel: will use Topology." << sendl;
msg_info() << "VisualModel: will use Topology.";
Copy link
Contributor

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.

damienmarchal
damienmarchal reviewed Nov 8, 2017
View reviewed changes
SofaKernel/modules/SofaBaseVisual/VisualModelImpl.cpp
@@ -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 << " )";
Copy link
Contributor

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:"

damienmarchal
damienmarchal reviewed Nov 8, 2017
View reviewed changes
SofaKernel/modules/SofaBaseVisual/VisualModelImpl.cpp Outdated
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;
Copy link
Contributor

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.

Copy link
Contributor Author

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

@damienmarchal
Copy link
Contributor

Thank Mr tablot for cleaning message.

I like this small but yet usefull PR.
I have added some suggestions to improve your PR a bit.

@hugtalbot hugtalbot added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Nov 14, 2017
@hugtalbot
Copy link
Contributor Author

hey @damienmarchal
I just tried with the macro at the top of the cpp file and I still get the error (for static function):
sofa/helper/logging/Messaging.h:177:24: note: expanded from macro 'msg_error' #define msg_error(...) MSGERROR_CHOOSER(__VA_ARGS__)(__VA_ARGS__)
Any idea why ?

@damienmarchal
Copy link
Contributor

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).

@hugtalbot
Copy link
Contributor Author

I confirm the macro is indeed after the includes and before the namespaces

@damienmarchal
Copy link
Contributor

The requirement for merged are met. What about passing to ready and merging ?

@guparan guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Nov 22, 2017
@damienmarchal
Copy link
Contributor

[ci-build]

@damienmarchal damienmarchal merged commit 107677f into sofa-framework:master Dec 5, 2017
@guparan guparan added this to the v17.12 milestone Dec 14, 2017
@hugtalbot hugtalbot deleted the clean_msgAPI_image_loading branch January 3, 2018 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guparan guparan guparan left review comments

@damienmarchal damienmarchal damienmarchal left review comments

@fredroy fredroy Awaiting requested review from fredroy

Assignees

@hugtalbot hugtalbot

Labels
enhancement About a possible enhancement pr: clean Cleaning the code pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Milestone
v17.12
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hugtalbot @guparan @damienmarchal