Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DIS 7 PDUs advertise zero length #68

Closed
Updownquark opened this issue Mar 9, 2020 · 4 comments
Closed

DIS 7 PDUs advertise zero length #68

Updownquark opened this issue Mar 9, 2020 · 4 comments
Labels

Comments

@Updownquark
Copy link

When marshalling instances of subclasses of edu.nps.moves.dis7.PduSuperclass, the length field is never initialized to anything but zero, so the marshalled PDU advertises length 0.

Further inspection reveals that this is also the case in the DIS 6 PDU types (the setPduLength() method is never called and the pduLength field is never set), but the marshalling code calls getLength(), which delegates to getMarshalledSize() (as opposed to getPduLength(), a getter for the field), calculating the PDU length instead of using the value of the length attribute.

It seems like the marshal call in DIS 7 should also be changed to use getMarshalledSize() or some delegate to it.

In both DIS 6 and 7 PDUs, the length field should probably be removed entirely. This is more robust than having to initialize a field anyway, especially for variable-length PDU types.

@leif81
Copy link
Member

leif81 commented Mar 11, 2020

Thank-you for the report @Updownquark .

Are you interested in submitting a pull request? Any help is really appreciated.

@Updownquark
Copy link
Author

Updownquark commented Mar 11, 2020

Sure, I can do that.

Would you like me to remove the length field from either or both Pdu abstract base classes as well, or just fix the bug? I could deprecate the getter and setter to avoid potential backward compatibility issues with any code that uses this.

FYI I forgot to mention that there's a fairly straightforward workaround for this bug, which is why I didn't fix it myself and submit the pull request already. Calling

pdu.setLength(pdu.getMarshalledSize())

before marshalling does the job.

@leif81
Copy link
Member

leif81 commented Mar 12, 2020

I think removing the pduLength field would be a good idea to remove confusion.

@leif81 leif81 added the bug label Mar 12, 2020
Updownquark pushed a commit to Updownquark/open-dis-java that referenced this issue Mar 12, 2020
…cating the getter and setter. Setter now does nothing, getter delegates to getMarshalledSize(). All uses of the field were replaced. Tested with EntityState and Acknowledge PDU types.
Updownquark pushed a commit to Updownquark/open-dis-java that referenced this issue Mar 12, 2020
…ecating the getter and setter. Setter now does nothing, getter delegates to getMarshalledSize(). All uses of the field were replaced. Tested with EntityState and Acknowledge PDU types.
Updownquark pushed a commit to Updownquark/open-dis-java that referenced this issue Mar 12, 2020
Updownquark pushed a commit to Updownquark/open-dis-java that referenced this issue Mar 12, 2020
Updownquark pushed a commit to Updownquark/open-dis-java that referenced this issue Mar 12, 2020
Updownquark pushed a commit to Updownquark/open-dis-java that referenced this issue Mar 12, 2020
@leif81
Copy link
Member

leif81 commented Mar 14, 2020

Fixed in #69

@leif81 leif81 closed this as completed Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants