From 4ccd5e3ec5d5ee4efef67f8bc57eac099d821015 Mon Sep 17 00:00:00 2001 From: Alex Bilger Date: Fri, 11 Sep 2026 09:15:01 +0200 Subject: [PATCH 1/4] [Core] Merge TMultiVec and its specialization for V_ALL --- .../framework/Core/src/sofa/core/MultiVecId.h | 366 +++++------------- 1 file changed, 93 insertions(+), 273 deletions(-) diff --git a/Sofa/framework/Core/src/sofa/core/MultiVecId.h b/Sofa/framework/Core/src/sofa/core/MultiVecId.h index 9ca45e2f380..aa76b4475f7 100644 --- a/Sofa/framework/Core/src/sofa/core/MultiVecId.h +++ b/Sofa/framework/Core/src/sofa/core/MultiVecId.h @@ -94,7 +94,7 @@ class TMultiVecId } public: bool hasIdMap() const { return idMap_ptr != nullptr; } - const IdMap& getIdMap() const + const IdMap& getIdMap() const { if (!idMap_ptr) { @@ -106,49 +106,53 @@ class TMultiVecId TMultiVecId() = default; - /// Copy from another VecId, possibly with another type of access, with the - /// constraint that the access must be compatible (i.e. cannot create - /// a write-access VecId from a read-only VecId. - template - TMultiVecId(const TVecId& v) - : - defaultId(v) + /// Copy from a TVecId. + /// When vtype != V_ALL: only the same vtype is accepted (vtype2 must equal vtype). + /// When vtype == V_ALL: any vtype2 is accepted (widening to V_ALL). + /// In both cases, write->read is allowed but read->write is forbidden. + template + requires (vtype == V_ALL || vtype2 == vtype) + TMultiVecId(const TVecId& v) : defaultId(v) { static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); } - /// Copy assignment from another VecId - template - TMultiVecId & operator= (const TVecId& v) { + /// Copy assignment from a TVecId (same constraints as the constructor above). + template + requires (vtype == V_ALL || vtype2 == vtype) + TMultiVecId& operator=(const TVecId& v) + { static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); defaultId = v; return *this; } - //// Copy constructor - TMultiVecId( const TMultiVecId& mv) - : defaultId( mv.getDefaultId() ) - , idMap_ptr( mv.idMap_ptr ) + //// Copy constructor (exact same type) + TMultiVecId(const TMultiVecId& mv) + : defaultId(mv.getDefaultId()) + , idMap_ptr(mv.idMap_ptr) { } - /// Copy assignment - TMultiVecId & operator= (const TMultiVecId& mv) { + /// Copy assignment (exact same type) + TMultiVecId& operator=(const TMultiVecId& mv) + { defaultId = mv.getDefaultId(); idMap_ptr = mv.idMap_ptr; return *this; } - //// Only TMultiVecId< V_ALL , vaccess> can declare copy constructors with all - //// other kinds of TMultiVecIds, namely MultiVecCoordId, MultiVecDerivId... - //// In other cases, the copy constructor takes a TMultiVecId of the same type - //// ie copy construct a MultiVecCoordId from a const MultiVecCoordId& or a - //// ConstMultiVecCoordId&. Other conversions should be done with the - //// next constructor that can only be used if requested explicitly. - template< VecAccess vaccess2> - TMultiVecId( const TMultiVecId& mv) : defaultId( mv.getDefaultId() ) + //// Copy constructor from a TMultiVecId of the same vtype but different access. + //// Only available when vtype != V_ALL. + //// For the vtype == V_ALL case, any-vtype2 widening is handled by the next + //// constructor instead. + //// When the access is compatible (write -> read), the id map is shared + //// instead of copied, because these types are binary-compatible. + template + requires (vtype != V_ALL && vaccess2 != vaccess) + TMultiVecId(const TMultiVecId& mv) : defaultId(mv.getDefaultId()) { - static_assert( vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden." ); + static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); if (mv.hasIdMap()) { // When we assign a V_WRITE version to a V_READ version of the same type, which are binary compatible, @@ -167,17 +171,15 @@ class TMultiVecId } template - TMultiVecId & operator= (const TMultiVecId& mv) { - static_assert( vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden." ); + requires (vtype != V_ALL && vaccess2 != vaccess) + TMultiVecId& operator=(const TMultiVecId& mv) + { + static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); defaultId = mv.defaultId; - if (mv.hasIdMap()) { - // When we assign a V_WRITE version to a V_READ version of the same type, which are binary compatible, - // share the maps like with a copy constructor, because otherwise a simple operation like passing a - // MultiVecCoordId to a method taking a ConstMultiVecCoordId to indicate it won't modify it - // will cause a temporary copy of the map, which this define was meant to avoid! - - // Type-punning + if (mv.hasIdMap()) + { + // Type-punning (same rationale as the constructor above) union { const std::shared_ptr< IdMap > * this_map_type; const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; @@ -189,266 +191,85 @@ class TMultiVecId return *this; } - //// Provides explicit conversions from MultiVecId to MultiVecCoordId/... - //// The explicit keyword forbid the compiler to use it automatically, as - //// the user should check the type of the source vector before using this - //// conversion. - template< VecAccess vaccess2> - explicit TMultiVecId( const TMultiVecId& mv) : defaultId( static_cast(mv.getDefaultId()) ) + //// Copy constructor from any TMultiVecId. + //// Only available when vtype == V_ALL (widening from a specific type to V_ALL). + //// The id map is shared instead of copied when the access direction is + //// compatible, for the same performance reason as above. + template + requires (vtype == V_ALL && (vtype2 != V_ALL || vaccess2 != vaccess)) + TMultiVecId(const TMultiVecId& mv) : defaultId(mv.getDefaultId()) { - static_assert( vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden." ); + static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); if (mv.hasIdMap()) { - IdMap& map = writeIdMap(); - - for (typename TMultiVecId::IdMap_const_iterator it = mv.getIdMap().begin(), itend = mv.getIdMap().end(); - it != itend; ++it) - map[it->first] = MyVecId(it->second); + // Type-punning (same rationale as the constructor for same-vtype different-access) + union { + const std::shared_ptr< IdMap > * this_map_type; + const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; + } ptr; + ptr.other_map_type = &mv.idMap_ptr; + idMap_ptr = *(ptr.this_map_type); } } - template - TMultiVecId & operator= (const TMultiVecId& mv) { - static_assert( vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden." ); + template + requires (vtype == V_ALL && (vtype2 != V_ALL || vaccess2 != vaccess)) + TMultiVecId& operator=(const TMultiVecId& mv) + { + static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); - defaultId = static_cast(mv.defaultId); + defaultId = mv.defaultId; if (mv.hasIdMap()) { - IdMap& map = writeIdMap(); - - for (typename TMultiVecId::IdMap_const_iterator it = mv.getIdMap().begin(), itend = mv.getIdMap().end(); - it != itend; ++it) - map[it->first] = MyVecId(it->second); + // Type-punning (same rationale as the constructor for same-vtype different-access) + union { + const std::shared_ptr< IdMap > * this_map_type; + const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; + } ptr; + ptr.other_map_type = &mv.idMap_ptr; + idMap_ptr = *(ptr.this_map_type); } return *this; } - void setDefaultId(const MyVecId& id) + //// Provides explicit conversions from TMultiVecId to a specific-vtype + //// TMultiVecId (e.g. MultiVecId -> MultiVecCoordId). + //// Only available when vtype != V_ALL. + //// The explicit keyword forbids the compiler to use it automatically: the + //// caller must have checked the type of the source vector before narrowing. + template + requires (vtype != V_ALL) + explicit TMultiVecId(const TMultiVecId& mv) + : defaultId(static_cast(mv.getDefaultId())) { - defaultId = id; - } + static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); - template - void setId(const std::set& states, const MyVecId& id) - { - if (!states.empty()) + if (mv.hasIdMap()) { IdMap& map = writeIdMap(); - for (auto* state : states) - map[state] = id; - } - } - - void setId(const BaseState* s, const MyVecId& id) - { - IdMap& map = writeIdMap(); - map[s] = id; - } - - void assign(const MyVecId& id) - { - defaultId = id; - idMap_ptr.reset(); - } - - const MyVecId& getId(const BaseState* s) const - { - if (!hasIdMap()) return defaultId; - const IdMap& map = getIdMap(); - - IdMap_const_iterator it = map.find(s); - if (it != map.end()) return it->second; - else return defaultId; - } - - const MyVecId& getDefaultId() const - { - return defaultId; - } - - std::string getName() const - { - if (!hasIdMap()) - return defaultId.getName(); - else - { - std::ostringstream out; - out << '{'; - out << defaultId.getName() << "[*"; - const IdMap& map = getIdMap(); - MyVecId prev = defaultId; - for (IdMap_const_iterator it = map.begin(), itend = map.end(); it != itend; ++it) - { - if (it->second != prev) // new id - { - out << "],"; - if (it->second.getType() == defaultId.getType()) - out << it->second.getIndex(); - else - out << it->second.getName(); - out << '['; - prev = it->second; - } - else out << ','; - if (it->first == nullptr) out << "nullptr"; - else - out << it->first->getName(); - } - out << "]}"; - return out.str(); - } - } - friend inline std::ostream& operator << ( std::ostream& out, const TMultiVecId& v ) - { - out << v.getName(); - return out; - } - - static TMultiVecId null() { return TMultiVecId(MyVecId::null()); } - bool isNull() const - { - if (!this->defaultId.isNull()) return false; - if (hasIdMap()) - for (IdMap_const_iterator it = getIdMap().begin(), itend = getIdMap().end(); it != itend; ++it) - if (!it->second.isNull()) return false; - return true; - } - - template - StateVecAccessor operator[](State* s) const - { - return StateVecAccessor(s,getId(s)); - } - - template - StateVecAccessor operator[](const State* s) const - { - return StateVecAccessor(s,getId(s)); - } -}; - - - -template -class TMultiVecId -{ -public: - typedef TVecId MyVecId; - - typedef std::map IdMap; - typedef typename IdMap::iterator IdMap_iterator; - typedef typename IdMap::const_iterator IdMap_const_iterator; - -protected: - MyVecId defaultId; - -private: - std::shared_ptr< IdMap > idMap_ptr; - - template friend class TMultiVecId; - -protected: - IdMap& writeIdMap() - { - if (!idMap_ptr) - idMap_ptr.reset(new IdMap()); - else if(!(idMap_ptr.use_count() == 1)) - idMap_ptr.reset(new IdMap(*idMap_ptr)); - return *idMap_ptr; - } -public: - bool hasIdMap() const { return idMap_ptr != nullptr; } - const IdMap& getIdMap() const - { - if (!idMap_ptr) - { - static const IdMap empty; - return empty; + for (typename TMultiVecId::IdMap_const_iterator it = mv.getIdMap().begin(), itend = mv.getIdMap().end(); + it != itend; ++it) + map[it->first] = MyVecId(it->second); } - return *idMap_ptr; } - TMultiVecId() = default; - - /// Copy from another VecId, possibly with another type of access, with the - /// constraint that the access must be compatible (i.e. cannot create - /// a write-access VecId from a read-only VecId. - template - TMultiVecId(const TVecId& v) : defaultId(v) + template + requires (vtype != V_ALL) + TMultiVecId& operator=(const TMultiVecId& mv) { static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); - } - - /// Copy assignment from another VecId - template - TMultiVecId & operator= (const TVecId& v) { - static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); - defaultId = v; - return *this; - } - - //// Copy constructor - TMultiVecId( const TMultiVecId& mv) - : defaultId( mv.getDefaultId() ) - , idMap_ptr( mv.idMap_ptr ) - { - } - - /// Copy assignment - TMultiVecId & operator= (const TMultiVecId& mv) { - defaultId = mv.getDefaultId(); - idMap_ptr = mv.idMap_ptr; - return *this; - } - - //// Only TMultiVecId< V_ALL , vaccess> can declare copy constructors with all - //// other kinds of TMultiVecIds, namely MultiVecCoordId, MultiVecDerivId... - //// In other cases, the copy constructor takes a TMultiVecId of the same type - //// ie copy construct a MultiVecCoordId from a const MultiVecCoordId& or a - //// ConstMultiVecCoordId&. - template< VecType vtype2, VecAccess vaccess2> - TMultiVecId( const TMultiVecId& mv) : defaultId( mv.getDefaultId() ) - { - static_assert( vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden." ); + defaultId = static_cast(mv.defaultId); if (mv.hasIdMap()) { - // When we assign a V_WRITE version to a V_READ version of the same type, which are binary compatible, - // share the maps like with a copy constructor, because otherwise a simple operation like passing a - // MultiVecCoordId to a method taking a ConstMultiVecCoordId to indicate it won't modify it - // will cause a temporary copy of the map, which this define was meant to avoid! - - // Type-punning - union { - const std::shared_ptr< IdMap > * this_map_type; - const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; - } ptr; - ptr.other_map_type = &mv.idMap_ptr; - idMap_ptr = *(ptr.this_map_type); - } - } - - template - TMultiVecId & operator= (const TMultiVecId& mv) { - static_assert( vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden." ); - - defaultId = mv.defaultId; - if (mv.hasIdMap()) { - // When we assign a V_WRITE version to a V_READ version of the same type, which are binary compatible, - // share the maps like with a copy constructor, because otherwise a simple operation like passing a - // MultiVecCoordId to a method taking a ConstMultiVecCoordId to indicate it won't modify it - // will cause a temporary copy of the map, which this define was meant to avoid! + IdMap& map = writeIdMap(); - // Type-punning - union { - const std::shared_ptr< IdMap > * this_map_type; - const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; - } ptr; - ptr.other_map_type = &mv.idMap_ptr; - idMap_ptr = *(ptr.this_map_type); + for (typename TMultiVecId::IdMap_const_iterator it = mv.getIdMap().begin(), itend = mv.getIdMap().end(); + it != itend; ++it) + map[it->first] = MyVecId(it->second); } return *this; @@ -530,13 +351,13 @@ class TMultiVecId } } - friend inline std::ostream& operator << ( std::ostream& out, const TMultiVecId& v ) + friend inline std::ostream& operator<<(std::ostream& out, const TMultiVecId& v) { out << v.getName(); return out; } - static TMultiVecId null() { return TMultiVecId(MyVecId::null()); } + static TMultiVecId null() { return TMultiVecId(MyVecId::null()); } bool isNull() const { if (!this->defaultId.isNull()) return false; @@ -547,17 +368,16 @@ class TMultiVecId } template - StateVecAccessor operator[](State* s) const + StateVecAccessor operator[](State* s) const { - return StateVecAccessor(s,getId(s)); + return StateVecAccessor(s,getId(s)); } template - StateVecAccessor operator[](const State* s) const + StateVecAccessor operator[](const State* s) const { - return StateVecAccessor(s,getId(s)); + return StateVecAccessor(s,getId(s)); } - }; From 2026d7f05e2f74bb7da1ef91961c71967da744a7 Mon Sep 17 00:00:00 2001 From: Alex Bilger Date: Fri, 11 Sep 2026 09:35:58 +0200 Subject: [PATCH 2/4] factorize type-punning --- .../framework/Core/src/sofa/core/MultiVecId.h | 56 ++++++++----------- 1 file changed, 22 insertions(+), 34 deletions(-) diff --git a/Sofa/framework/Core/src/sofa/core/MultiVecId.h b/Sofa/framework/Core/src/sofa/core/MultiVecId.h index aa76b4475f7..c2401bcfb2b 100644 --- a/Sofa/framework/Core/src/sofa/core/MultiVecId.h +++ b/Sofa/framework/Core/src/sofa/core/MultiVecId.h @@ -81,7 +81,24 @@ class TMultiVecId private: std::shared_ptr< IdMap > idMap_ptr; - template friend class TMultiVecId; + /// Share the id map from a binary-compatible TMultiVecId instantiation + /// without copying it. Safe because TVecId and + /// TVecId have the same memory layout (same underlying + /// integral index; access direction is a type-level tag only). + /// This avoids an O(n) map copy when e.g. a writable id is passed to a + /// function expecting a read-only one. + template + void sharedIdMapCast(const TMultiVecId& mv) + { + union { + const std::shared_ptr* this_map_type; + const std::shared_ptr::IdMap>* other_map_type; + } ptr; + ptr.other_map_type = &mv.idMap_ptr; + idMap_ptr = *(ptr.this_map_type); + } + + template friend class TMultiVecId; protected: IdMap& writeIdMap() @@ -155,18 +172,7 @@ class TMultiVecId static_assert(vaccess2 >= vaccess, "Copy from a read-only multi-vector id into a read/write multi-vector id is forbidden."); if (mv.hasIdMap()) { - // When we assign a V_WRITE version to a V_READ version of the same type, which are binary compatible, - // share the maps like with a copy constructor, because otherwise a simple operation like passing a - // MultiVecCoordId to a method taking a ConstMultiVecCoordId to indicate it won't modify it - // will cause a temporary copy of the map, which this define was meant to avoid! - - // Type-punning - union { - const std::shared_ptr< IdMap > * this_map_type; - const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; - } ptr; - ptr.other_map_type = &mv.idMap_ptr; - idMap_ptr = *(ptr.this_map_type); + sharedIdMapCast(mv); } } @@ -179,13 +185,7 @@ class TMultiVecId defaultId = mv.defaultId; if (mv.hasIdMap()) { - // Type-punning (same rationale as the constructor above) - union { - const std::shared_ptr< IdMap > * this_map_type; - const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; - } ptr; - ptr.other_map_type = &mv.idMap_ptr; - idMap_ptr = *(ptr.this_map_type); + sharedIdMapCast(mv); } return *this; @@ -203,13 +203,7 @@ class TMultiVecId if (mv.hasIdMap()) { - // Type-punning (same rationale as the constructor for same-vtype different-access) - union { - const std::shared_ptr< IdMap > * this_map_type; - const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; - } ptr; - ptr.other_map_type = &mv.idMap_ptr; - idMap_ptr = *(ptr.this_map_type); + sharedIdMapCast(mv); } } @@ -222,13 +216,7 @@ class TMultiVecId defaultId = mv.defaultId; if (mv.hasIdMap()) { - // Type-punning (same rationale as the constructor for same-vtype different-access) - union { - const std::shared_ptr< IdMap > * this_map_type; - const std::shared_ptr< typename TMultiVecId::IdMap > * other_map_type; - } ptr; - ptr.other_map_type = &mv.idMap_ptr; - idMap_ptr = *(ptr.this_map_type); + sharedIdMapCast(mv); } return *this; From 41da06c3d8615f4ad77c54e849df9402ba657798 Mon Sep 17 00:00:00 2001 From: Alex Bilger Date: Fri, 11 Sep 2026 09:40:35 +0200 Subject: [PATCH 3/4] modernize loops --- Sofa/framework/Core/src/sofa/core/MultiVecId.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Sofa/framework/Core/src/sofa/core/MultiVecId.h b/Sofa/framework/Core/src/sofa/core/MultiVecId.h index c2401bcfb2b..1d17e7a8265 100644 --- a/Sofa/framework/Core/src/sofa/core/MultiVecId.h +++ b/Sofa/framework/Core/src/sofa/core/MultiVecId.h @@ -238,9 +238,10 @@ class TMultiVecId { IdMap& map = writeIdMap(); - for (typename TMultiVecId::IdMap_const_iterator it = mv.getIdMap().begin(), itend = mv.getIdMap().end(); - it != itend; ++it) - map[it->first] = MyVecId(it->second); + for (const auto& [k, v] : mv.getIdMap()) + { + map.insert(std::make_pair(k, MyVecId(v))); + } } } @@ -255,9 +256,10 @@ class TMultiVecId { IdMap& map = writeIdMap(); - for (typename TMultiVecId::IdMap_const_iterator it = mv.getIdMap().begin(), itend = mv.getIdMap().end(); - it != itend; ++it) - map[it->first] = MyVecId(it->second); + for (const auto& [k, v] : mv.getIdMap()) + { + map.insert(std::make_pair(k, MyVecId(v))); + } } return *this; From 08ceb7317561efe3ed4d430bfab1a7fe83305aba Mon Sep 17 00:00:00 2001 From: Alex Bilger Date: Fri, 11 Sep 2026 09:47:34 +0200 Subject: [PATCH 4/4] modernize isNull --- Sofa/framework/Core/src/sofa/core/MultiVecId.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Sofa/framework/Core/src/sofa/core/MultiVecId.h b/Sofa/framework/Core/src/sofa/core/MultiVecId.h index 1d17e7a8265..b6afbab9fa0 100644 --- a/Sofa/framework/Core/src/sofa/core/MultiVecId.h +++ b/Sofa/framework/Core/src/sofa/core/MultiVecId.h @@ -352,8 +352,10 @@ class TMultiVecId { if (!this->defaultId.isNull()) return false; if (hasIdMap()) - for (IdMap_const_iterator it = getIdMap().begin(), itend = getIdMap().end(); it != itend; ++it) - if (!it->second.isNull()) return false; + { + const auto& idMap = getIdMap(); + return std::all_of(idMap.begin(), idMap.end(), [](const auto& el){return el.second.isNull();}); + } return true; }