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

Fixes and improvements for the MMPLD writer #280

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

cniethammer
Copy link
Contributor

@cniethammer cniethammer commented Dec 8, 2023

Description

Fixes and minor improvements for the MMPLD writer:

  • Fix incorrect last entry in seek table
  • Add sanity check for running on little endian systems
  • Use named macro constant instead of meaningless integer values in code

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • running EOX example and checking mmpld file output with the Megamol mmpldinfo.py script

cniethammer and others added 3 commits December 7, 2023 17:12
The MMPLD file format includes a seek table for the data frames.
The last entry of this table should point at the end of the last
frame. However, this is not identical to the file size in some cases.
Correct seek table entries are already inserted by the write_frame()
method. Therefore, removing the erroneous code in the finish() method.

Signed-off-by: Christoph Niethammer <[email protected]>
Co-authored-by: HomesGH <[email protected]>
The MMPLD file format (version 1.2) is little endian only. The MMPLD writer
uses simple byte writes without conversions so we have to make sure that
we are running on a little endian system.

Signed-off-by: Christoph Niethammer <[email protected]>
@cniethammer cniethammer requested a review from HomesGH December 8, 2023 09:46
@cniethammer cniethammer added bug Something isn't working and removed bug Something isn't working labels Dec 8, 2023
Copy link
Contributor

@HomesGH HomesGH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but there is still the problem with the restart.
For clarity, I added another example to the test section in the description of this PR. This one does not perfectly work when having a look at the resulting mmpld-file with the mmpldinfo.py tool.

@cniethammer
Copy link
Contributor Author

I am not sure if this problem is related to the fixes in this PR as mmpldinfo.py reported for me also errors before on the surface-tension example.
I would like to proceed with merging these small fixes instead of holding them due to another issue.

@HomesGH
Copy link
Contributor

HomesGH commented Dec 15, 2023

I am not sure if this problem is related to the fixes in this PR as mmpldinfo.py reported for me also errors before on the surface-tension example.
I would like to proceed with merging these small fixes instead of holding them due to another issue.

Ok, alright. At least those bugs are fixed then ;)

@cniethammer cniethammer merged commit 98d77b7 into ls1mardyn:master Dec 18, 2023
52 checks passed
@cniethammer cniethammer deleted the fix-MmpldWriter branch February 13, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants