Conversation
|
|
||
| uint32_t express::Base::id() const { return data()->id(); } | ||
|
|
||
| const InstanceData* express::Base::data() const { |
There was a problem hiding this comment.
This method, as well as the non-const version, defeats using smart pointers. Once the raw pointer is returned it is no longer managed and could be deleted by the caller. Why not
auto sp = data_.lock();
if (sp) {
return sp;
} else {
throw std::runtime_error("Trying to access deleted instance reference")
}
There was a problem hiding this comment.
That's true, but I have the feeling that maybe this should be marked private instead and then it doesn't really matter anymore.
|
Does the weak_ptr encapsulated in the express::Base retain file scope lifetime of objects? It seems like shared_ptr would be required. Though, I don't have a complete understanding. Creating entities as part of the file without constructors has a trade-off. Instead of create() + add() a user of the SDK must do create() + multiple parameter initialization steps by calling set() methods. This isn't bad, but it has these consequences:
I very much like eliminating the custom classes for aggregates. I found them difficult to use. It was difficult to know which one of the classes where needed in different situations. Their interface wasn't the same as std collections, so looping wasn't as easy and they couldn't be used in the std algorithms. This is going to break a lot of my code, but the fixes appear to be easy to deal with. @aothms Let's set up a time for you to walk me through the bigger picture. I'm sure there are lots of details that I'm missing. |
|
|
||
| // Disable warnings coming from IfcOpenShell | ||
| #pragma warning(disable : 4018 4267 4250 4984 4985) | ||
| /******************************************************************************** |
There was a problem hiding this comment.
It looks like a different example has been applied over the IfcAlignment.cpp example. The revised file is significantly different. Is this intentional?
There was a problem hiding this comment.
Sorry that was sloppy, it's back and updated for the code changes
Yes the file have the shared_ptr not so much for sharing ownership but just so that we have the option to have a weak_ptr derived from them. So that when you have instances pointing to a deleted file or to instances from the file that have been deleted.
Yes that's very valid. The discovery is indeed not really there. I'm also fine with forwarding the arguments from file.create and reinstate the constructor. But as I said since the arguments are not named I find them a bit unclear especially with all the std::nullopts for all the optional arguments, it's hard to interpret. Boost has this named parameters idea, but I think it's a bit lengthy to setup https://www.boost.org/doc/libs/latest/libs/graph/doc/bgl_named_params.html
I think that's indeed also a nice compromise. Then we also have autocompletion in our IDE because I think with std::forward from file.create() I think IDE's will be pretty clueless. Very flexible to have a call these days, let me know some times that suit you please. |
|
I emailed your personal account with some proposed times for next week |
|
For the pointer issue, should it be Note I also have these two hacks applied:
Explanation in findings.md |
|
Some Python workarounds whilst things are moving: eea2398 Also I have confirmed for RDB there seem to be indeterministic shape counts: import sys
# sys.path.insert(0, "/home/dion/Projects/ios-dm/src/ifcopenshell-python")
# sys.path.insert(0, "/home/dion/Projects/ios-dm/build/ifcwrap")
sys.path.insert(0, "./build/Linux/x86_64/install/ifcopenshell")
import multiprocessing
import ifcopenshell
import ifcopenshell.geom
f = ifcopenshell.open("/home/dion/Projects/ios-dm/models/a.rdb")
print(f"schema: {f.schema}")
print(f"projects: {len(f.by_type('IfcProject'))}")
print(f"contexts: {len(f.by_type('IfcGeometricRepresentationContext'))}")
print(f"reps: {len(f.by_type('IfcRepresentation'))}")
settings = ifcopenshell.geom.settings()
it = ifcopenshell.geom.iterator(settings, f, multiprocessing.cpu_count(), geometry_library="opencascade")
ok = it.initialize()
print(f"initialize (opencascade): {ok}")
if ok:
n = 1
while it.next():
n += 1
print(f" shapes: {n}")With cpu count 1 I always get 107 shapes. With all CPUs, I get different numbers (101, 103, etc). I guess the mutex thing had some truth to it? |
For the past couple of days I have been rethinking what a sane v1.0 data model on the C++ side would look like that balances safety, readability, performance and a bit more modern language features. I still have work todo but wanted to share a bit of what's going on.
This has huge implications on client C++ code, so to feel the pain I went through most of the code base already. @RickBrice as one of the more active C++ devs lately I'm curious what your thoughts are.
Outcome:
Before:
After:
Rationale
reason for weak_ptr encapsulated in object:
reason for creation as part of file
reasons for no constructor:
no custom class anymore for aggregates (aggregate_of_instance)