Closed AnyOldName3 closed 6 months ago
I am away this weekend, but on my return will have a bash at reproducing this issue and testing out the proposed fix.
What is the best way to reproduce the issue?
Apart from simply looking at the code, you could use the same builder instance to create several identical subgraphs, and then inspect things in a debugger and see that they're distinct (except for the parts that the separate SharedObjects
instance has deduplicated), and that the builder's GeometryMap
members remain empty.
Once that's fixed, you can demonstrate that the StateInfo
needs to be part of the GeometryMap
key by asking the builder to create two shapes with the same geometry but different state. Instead, the cached instance from the first call will be returned, and both calls will give results that use the same state.
Do you not have code that reproduces the issue?
If i was to write my own code/modify other examples based on what I think you mean I won't know for sure that it's what you have in mind.
Not exactly - I noticed the problems when making a subclass of vsg::Builder
for vsgPhysX that took physx::PxGeometry
subclasses instead of vsg::GeomInfo
and looking at the code rather than running into it directly. I noticed that nothing was ever replacing the null pointers for my subclass, and checked and the problem existed in the original implementation, too.
As a quick demo, I just tried throwing
std::array<vsg::ref_ptr<vsg::Node>, 4> nodes;
for (auto& node : nodes)
node = builder->createQuad(geomInfo, stateInfo);
into the vsganimation
example, and get four different pointers back with master, and the same pointer four times with my PR.
For the second problem, when the half of my PR that enables the cache, but not the half that adds the StateInfo
to the key is present,
std::array<vsg::ref_ptr<vsg::Node>, 4> nodes;
for (auto& node : nodes)
{
stateInfo.two_sided = (size_t(&node) / sizeof(node)) % 2;
node = builder->createQuad(geomInfo, stateInfo);
}
gives the same node with the same state four times, and adding the second half of the PR makes it so the calls with identical state give the same pointer, but the calls with different state give different pointers.
When I get back I'll try out vsgPhysX and use it to understand why you are subclass and what is happening under the hood.
Just had a quick look at it vsgPhysX, choice of LGPL is odd choice given the rest of the VSG projects are MIT. Looking at 3rd party code and learning from it, then creating new code doesn't infringe on copyright, only directly copying code infringes and requires maintaining the license.
It's a grey area, with case law in both directions. It's okay to have one person read and understand the code and write a description in pseudocode, and have a second person implement something that does the same job, but when one person sees the original code and implements a replacement, it's dependent on the specifics of the case, the mood of the judge, and the persuasiveness of the lawyers. That said, when the licencing discussion happened, I was expecting to copy and paste a lot of stuff, and ended up not doing, so it's ended up more distinct than I'd expected, and might be worth reconsidering.
I have just started looking at building PhysX and vsgPhysX/osgPhysX. So far I haven't seen similarities between vsgPhysX and osgPhysX. I do see elements from the VSG which is fine but makes me perplexed that you've chosen LGPL over MIT when you are using code form a MIT project and not from the LGPL project. I don't expect the osgPhysX author Rui Wang would object to you using MIT for a project that isn't derived from osgPhysX.
My aim for this afternoon is resolve the vsg::Builder issue and get up to speed with vsgPhysX.
As I said, I was expecting to use more from osgPhysX than I ended up doing, and while an LGPL project can use MIT code, the reverse isn't true, so if I'd used enough of osgPhysX to count as a derivative work, the LGPL's the least restrictive licence that would have worked.
There are definitely similarities between vsgPhysX's Actor.cpp
and osgPhysX's PhysicsUtil.cpp
, but they're just what'd be the only obvious way to connect PhysX and something like the OSG or VSG, so there's a strong argument that the similarities would have been there had vsgPhysX been created without any prior knowledge of osgPhysX. I was looking at osgPhysX when I wrote it, though.
I have just reviewed the code. Having the same method names doesn't infringe and the implementation is wholly different, the similarities are just things like createBoxActor, but even then the return and parameter list and implementation are different. It's a huge stretch to imagine this is a derivative work.
The whole vsg-dev family are MIT licensed, specially to avoid the ambiguities that LGPL projects introduce. For vsgPhysX to become part of this vsg-dev family it really needs to conform to the same pattern that the other projects have established.
My recommendation is change to MIT. If there are files you are not 100% confident then contact Rui Wang to let him know of the work and desire to use MIT, he can then do a review and if there are issues we can then decide how to resolve them.
FYI, Rui Wang has said he's planning on using VSG projects this year so may well want to contribute to vsgPhysX as well as other projects.
I have done the code review the caches in vsg::Builder and yep they aren't populated. I presume I created this regression when I introduced the decorateAndCompileIfRequired() that was required to centralize a number of subgraph setup issues.
I will now review your changes and reflect on what the best way forward is to address this.
The node caches in
vsg::Builder
, i.e._boxes
and friends, are never populated, they're only queried, and obviously those queries fail because nothing's been put into them. My assumption is thatdecorateAndCompileIfRequired
used to be duplicated for each create function, as it's got a variable calledsubgraph
, and the cache query result is put in a variable with the same name, so if they'd previously been the same variable, the cache would have worked.Also, it looks like if this problem were fixed, the
StateInfo
would need to be part of the cache key, too, as it affects the result that's returned.