ThatOpen / engine_components

MIT License
349 stars 134 forks source link

FragmentGroup.uuid should not be tied to projectGuid #209

Closed vulevukusej closed 5 months ago

vulevukusej commented 11 months ago

Describe the bug 📝

It's common in larger projects to split up a project into smaller, seperate files. These file all have the same IfcProject and thus the same projectGuid. This causes an issue in components, since it seems the fragmentGroup uuid is taken from the project guid.

Reproduction ▶️

No response

Steps to reproduce 🔢

No response

System Info 💻

openbim-components

Used Package Manager 📦

npm

Error Trace/Logs 📃

No response

Validations ✅

HoyosJuan commented 11 months ago

Hey @vulevukusej,

You're totally right! It was a decision we made at the time to set the IfcProject GUID as the FragmentsGroup.uuid without taking into account the splitting of one authoring model into several IFC files (in fact, I do it quite often).

Sadly, at the time being the IFC schema doesn't come with a GUID to identify the model as seen in https://github.com/buildingSMART/NextGen-IFC/issues/67 (quite old issue but apparently still unresolved?). We basically had two options:

  1. Generate a GUID completely decoupled from the IFC file.
  2. Use the GUID from some root level entity. We choose IfcProject.

Being that said, you think we should stick to the first option? It will surely help avoid issues in many of the library tools that identify the model using FragmentsGroup.uuid. Also, do you have any other suggestion for this? If you don't, then we would go back to the autogenerated GUID decoupled from the IFC file. At least, if you export the Fragments their ID would remain the same.

Thank you so much!

vulevukusej commented 11 months ago

Hey @HoyosJuan - I'm not sure what the purpose of the FragmentsGroup.uuid is other than to uniquely it identify amongst other FragmentGroups? In that case, why do we need more complexity than the filename itself? I modified the code to make the uuid = filename and I've found it to work perfectly. If a user tries to import a model with the same filename this is rejected and the user is appropriately informed. An IfcProject refers to the project conceptually and not the file which is why a guid there makes much more sense to me. In any case, just my 2 cents :)

NiklasPor commented 11 months ago

I think it'd be nice if the UUID of the fragment group would stay stable, when the same ifc file is opened again later. Currently we're using expressId + fragmentGroup.uuid to uniquely identify elements inside models. (For example to save selections and restore them later when the model is opened again). How could one achieve that, when the fragment group id would be dynamically generated?

vulevukusej commented 11 months ago

@NiklasPor why not just use the object guid from IFC for that? It's guaranteed to be unique across projects

NiklasPor commented 11 months ago

Actually a good idea. I'd just have to build up a Map when a new model is loaded, which maps the element guids to the data we currently fetch with expressId + fragmentGroup.uuid 👍

(or search through all fragment groups with IfcPropertiesUtils.findItemByGuid(group.properties, guid) and cache the result. (Just checked the implementation, it also just iterates over all properties 😄 )`

vulevukusej commented 11 months ago

Yup that's what I did 👍

agviegas commented 5 months ago

In the latest versions we have decoupled the FragmentsGroup uuid from the project guid, so I'm closing this!