Skip to content

[Helper] Remove default value on OptionGroup #5605

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.

Sign up for GitHub

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

Closed
hugtalbot wants to merge 2 commits into from
Closed

[Helper] Remove default value on OptionGroup #5605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd1eb54
[Response] Clean init of CollisionResponse
hugtalbot Jul 10, 2025
cce4867
[Helper] Remove default value on OptionGroup
hugtalbot Jul 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions ...sion/Response/Contact/src/sofa/component/collision/response/contact/CollisionResponse.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,38 @@ sofa::helper::OptionsGroup CollisionResponse::initializeResponseOptions(sofa::co
}

sofa::helper::OptionsGroup responseOptions(listResponse);
if (listResponse.contains("PenalityContactForceField"))
responseOptions.setSelectedItem("PenalityContactForceField");

return responseOptions;
}

void CollisionResponse::init()
{

Inherit1::init();

if(!d_response.isSet())
{
msg_error() << "No response method has been set. Default response=\"PenalityContactForceField\"";
setDefaultResponseType("PenalityContactForceField");
return;
}

if (d_response.getValue().size() == 0)
{
msg_error() << "Response method is empty and may have been wrongly set. Option list is: " << initializeResponseOptions(getContext());
d_response.setValue(initializeResponseOptions(getContext()));
}
else
{
sofa::helper::OptionsGroup responseOptions = initializeResponseOptions(getContext());
if(!responseOptions.isInOptionsList(d_response.getValue().getSelectedItem()))
{
msg_error() << "response method is not a valid response method. Response can be among the list: " << responseOptions.getItemNames();
}
else
{
msg_info() << "Valid response method: " << d_response.getValue().getSelectedItem();
}
}
}

void CollisionResponse::cleanup()
Expand Down Expand Up @@ -111,7 +130,6 @@ void CollisionResponse::setDefaultResponseType(const std::string &responseT)
}
}


void CollisionResponse::changeInstance(Instance inst)
{
core::collision::ContactManager::changeInstance(inst);
Expand Down
1 change: 0 additions & 1 deletion ...lision/Response/Contact/src/sofa/component/collision/response/contact/CollisionResponse.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ public :
/// The number of contacts corresponds to the number of collision models
/// currently in contact with a collision model.
void setNumberOfContacts() const;

};


Expand Down
6 changes: 4 additions & 2 deletions Sofa/framework/Helper/src/sofa/helper/OptionsGroup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class OptionsGroup;
//////////////////////////////////////////////////////////////////////////////////////////////////////
OptionsGroup::OptionsGroup() : textItems()
{
selectedItem=0;
Copy link
Copy Markdown
Contributor

@bakpaul bakpaul Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certainly breaking. Even if not set here, selectedItem will have a default value. Might as well control it no ? Or add an internal mechanism that track if the optionGroup has been set at least once, to know if we can trust this attribute.

Copy link
Copy Markdown
Contributor

@alxbilger alxbilger Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. selectedItem will have a value if you define it or not. I don't see how it solves your problem. Could you imagine a fix in the CollisionResponse class instead?

}
///////////////////////////////////////
OptionsGroup::OptionsGroup(const OptionsGroup & m_radiotrick) : textItems(m_radiotrick.textItems)
Expand All @@ -42,12 +41,15 @@ OptionsGroup::OptionsGroup(const OptionsGroup & m_radiotrick) : textItems(m_radi
void OptionsGroup::setNbItems(const size_type nbofRadioButton )
{
textItems.resize( nbofRadioButton );
selectedItem = 0;
}
///////////////////////////////////////
void OptionsGroup::setItemName(const unsigned int id_item, const std::string& name )
{
textItems[id_item] = name;
}///////////////////////////////////////
type::vector<std::string> OptionsGroup::getItemNames()
{
return textItems;
}
///////////////////////////////////////
int OptionsGroup::isInOptionsList(const std::string & tempostring) const
Expand Down
3 changes: 3 additions & 0 deletions Sofa/framework/Helper/src/sofa/helper/OptionsGroup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ public :
///Set the name of the id-th item
void setItemName( unsigned int id_item, const std::string& name );

///Get the vector of names available
type::vector<std::string> getItemNames();
Copy link
Copy Markdown
Contributor

@bakpaul bakpaul Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any usage of this newly introduced method.


template <class T>
void setNames(const std::initializer_list<T>& list);

Expand Down
Loading