Avoid duplicate resource ids #386
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #386 +/- ##
========================================
Coverage 71.10% 71.11%
========================================
Files 271 271
Lines 30307 30317 +10
========================================
+ Hits 21551 21561 +10
Misses 8756 8756 ☔ View full report in Codecov by Sentry. |
Source/Model/Classes/NMR_Model.cpp
Outdated
| ModelResourceID CModel::generateResourceID() | ||
| { | ||
| // Build a map with existing resource IDs | ||
| std::map<ModelResourceID, bool> existingResourceIDs; |
There was a problem hiding this comment.
this could be a std::unordered_set. since we dont use the bool value.
There was a problem hiding this comment.
@gangatp unordered_set won't sort the ModelResourceID's right ?
I am picking the new id from the map using a reverse iterator.
Would you be okay with std::set instead ?
There was a problem hiding this comment.
I fear this is a major bottleneck as it will build up the map every time "generateresourceID" is called.
It would be much easier to store a m_MaxResourceID in the model that is updated whenever we add a resource.
There was a problem hiding this comment.
@alexanderoster I thought about this too. I can just populate it once and then simply reuse it or just store a m_MaxResourceID. m_Resources doesn't change after a model is read. So, it can be only read once and then reused.
There was a problem hiding this comment.
I think I was wrong in assuming this.. i will implement a m_MaxResourceID instead.
|
@gangatp @alexanderoster I have made the changes. Kindly review and let me know if this is what you were looking for. |
| else | ||
| return 1; | ||
| } | ||
| // Ensure the generated ID is unique |
There was a problem hiding this comment.
Do we need to update m_maxResourceId here, if currentResourceId > m_maxResourceId ?
There was a problem hiding this comment.
Tests pass with this. m_MaxResourceID tracks the maximum of package id only. There is only a single collision scenario when ids don't start from 1.
I think incrementing m_maxResourceID here would make it skip even more ids.
There was a problem hiding this comment.
This is what we are doing at our internal version at HP
ModelResourceID CModel::generateResourceID()
{
// TODO: is this truly safe?
ModelResourceID ret{ 0 };
for ( const auto & it : m_ResourceMap )
{
ret = std::max(ret, it.second->getPackageResourceID()->getModelResourceID());
}
return ret + 1;
}
Thanks for sharing. We are implementing in a similar way. |
This PR addresses the following issues:
#380
#355
#183
In Microsoft 3D Builder, object IDs seem to start from 2, which affects the
CModel::generateResourceID()function, causing it to generate duplicate resource IDs. This PR fixes the issue by checking the model resource IDs from the package and incrementing them accordingly to avoid collisions. Current tests show that it maintains the OPC package IDs correctly, incrementing them as needed. As a result, users will not notice any difference when the file is written out.Feedback is welcome.