Update to disco to be more compliant with FOM module loading #68
Conversation
Disco was previously throwing an exception if two FOM modules defined the same parameter for a class. However according to the spec, this is fine, as long as the definition is the same. This now allows duplicate parameters. NOTE: currently assuming that parameters with the same name ARE the same, which is not necessarily the case.
|
@michaelrfraser these are the updates for getting the VR Forces FOM modules to load. I haven't tested outside of checking CNR starts up - so probably don't want to actually merge this (at least not yet) - just putting it in a PR so you can see the changes |
| "hla/rpr2/RPR-Warfare_v2.0.xml" | ||
| "hla/rpr2/RPR-Warfare_v2.0.xml", | ||
| "hla/rpr2/RPR-Aggregate_v2.0.xml", | ||
| "hla/rpr2/RPR-SE_v2.0.xml", |
There was a problem hiding this comment.
Are these extra files already included in the codebase?
There was a problem hiding this comment.
Yeah... I don't know why they weren't included (perhaps we just never used anything in them?)
There was a problem hiding this comment.
These shouldn't be needed by Disco and should be removed. Any module that is not 100% required (because there is a mapper that uses data from it) should be excluded. If other federates/applications need these models they can add them within the scope of their applications.
| name, parameter.getName() ); | ||
| // FIXME - we'll assume that this parameter has the same definition - which makes this fine | ||
| // but if it were defined any differently we should error out. | ||
| return; |
There was a problem hiding this comment.
Ideally this should have equivalency checks on the parameters as per portico. Are we just treating this current change as a stop-gap to get current work out of the way?
There was a problem hiding this comment.
Yep exactly. I think the way DISCO is doing this merging would need to change somewhat fundamentally for us to be able to do such equivalency checks - as far as I can tell - the only info we have about parameters at this point is the name, so would need to rework so that we have the full definition of the existing and the incoming parameters.
There was a problem hiding this comment.
Disco isn't doing any FOM merging. It is reading the collective set of provided FOM modules to cache information from it so that it can reference it later. The only information required to be stored for Interactions is the name I believe, because it's the only thing from the FOM that Disco uses in operation (gets used in pubsub). I believe this check by itself should just be enough.
There was a problem hiding this comment.
No, I lie - Parameters are also needed (for handle lookup).
This will need to expand to just loop over all params and add them to the set of cached parameters. If the RTI allows the set of FOM modules then you know they should comply with any HLA merging rules, so there should be no issue with just cumulatively adding to the store of parameters. That is effectively what happens, so it should be OK.
Disco was previously throwing an exception if two FOM modules defined the same parameter for a class. However according to the spec, this is fine, as long as the definition is the same. This now allows duplicate parameters.
NOTE: currently assuming that parameters with the same name ARE the same, which is not necessarily the case.