-
Notifications
You must be signed in to change notification settings - Fork 12
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
Change ScalarType
to U8
for colors in lookup tables stored in binary files
#47
Change ScalarType
to U8
for colors in lookup tables stored in binary files
#47
Conversation
This looks like a bug, nice find and thank you for the PR!
We will need to parse as float for ASCII and u8 for binary. Could you update the PR to do this and add a test? I would be happy to accept. |
Heyo 👋 I've been looking into this issue together with @Siimeloni and we've found a couple of things that probably make this fix a bit more involved. We wanted to summarize our findings so far to maybe get some input from @elrnv and to just have something to come back to in the future. Changing the parser part is only half of the required fix here as we also need to adjust the writer side. We have mostly looked at the Here we run into the first issue. Since the Then I started to look into the writer code more deeply. I guess that, up until this point, the assumptions under which the VTK model and the parser/writer infrastructure haven been devised include that all attributes for a given dataset always remain in the same type independent of the input/output format. The writer functions are relatively rigid in regards to the output type. They just write whatever buffer they have been fed. For this specific lookup table we would have to decide whether to write Since I wanted to try something anyway, I hacked something into the writer code (adjusting Lines 881 to 883 in 02d9100
F32 input to U8 if the length would match the lookup table data from the test. This leads to the writer/parser combination working nicely, but of course that is not a practical solution. Additionally, the test still would not work because a difference in the VTK Model (test expects F32 , but now we parse a U8 buffer).
This might be another issue that came up. With the changes described above, the VTK model might contain an F32 -> F32 I'm not sure how practical it is that the VTK model is directly dependent on the input data type at this point. Maybe it would make sense to alway convert this lookup table data to either Best regards |
This is implemented in the writer and parser.
Good point. I added an writer implementation here: GiGainfosystems#1 . Please review to see if I missed anything. |
I've incorporated the remaining changes discussed on the other PR and I think this should be ready now. Thanks again for your input and work on this! |
Just one comment above. Otherwise ready to merge. Thank you, @MrMuetze, for adding these changes here! |
Cool, Im not sure what comment you are referring to though. 😅 Am I not seeing something on my end? |
src/writer.rs
Outdated
@@ -869,8 +863,8 @@ mod write_vtk_impl { | |||
} | |||
|
|||
match buf { | |||
IOBuffer::Bit(v) => write_buf_impl(v, &mut self.0, |x| x * 255 as u8)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we change this to if x != 0 { 255 } else { 0 }
to make it explicit. I think all of these other than u8 and f32 are just trying to predict the intent of the user who may have made a mistake. Since Bit is either zero or 1, I think it's more appropriate to have 1 correspond to the largest byte value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this doesn't apply anymore. I've removed the * 255
part here (see GiGainfosystems#1 (comment)). My reasoning was that the bit case also includes a Vec<u8>
(which is a bit curious). So I handled it like the other match arm.
pub enum IOBuffer {
/// Bit array is stored in 8 bit chunks.
Bit(Vec<u8>),
...
}
So for this type of conversion only the float values need to be multiplied by 255.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, you are absolutely right, thank you for that reference!
sorry about that, I forgot to submit the review! Do you see the comment now? |
When trying to parse a binary .vtk file the program had an error in the
Vtk::parse_legacy_be()
function.The file has following structure:
The error happens in the
attribute_lookup_table()
function.It was not possible to parse this file because
vtkio
was expecting the color values (r, g, b, a) to be encoded in f32 binary and not in u8. Since we are able to write these files on our own we tested output with the color values written as f32, but on import ParaView would throw an error. This does not happen with the u8 color values, so we assume that u8 is the correct encoding here.When changing the appropriate location in vtkio we are able to successfully parse these files.