-
Notifications
You must be signed in to change notification settings - Fork 9
Use correct field size for t8_vtk_data_field_t #126
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
base: main
Are you sure you want to change the base?
Changes from all commits
f231c85
9bfeb0d
853b481
6ada26b
f8f67d2
00ef6bb
01afd3e
5db9020
688db5d
cf650db
fc5aa15
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -258,6 +258,26 @@ macro T8_ASSERT(q) | |||||||||||||||||
| :($(esc(q)) ? nothing : throw(AssertionError($(string(q))))) | ||||||||||||||||||
| end | ||||||||||||||||||
|
|
||||||||||||||||||
| # platform specific BUFSIZ used in t8_vtk_data_field_t | ||||||||||||||||||
| # TODO: Just a guess! | ||||||||||||||||||
| if Sys.isapple() | ||||||||||||||||||
| const T8_BUFSIZ = 1024 | ||||||||||||||||||
| elseif Sys.iswindows() | ||||||||||||||||||
| const T8_BUFSIZ = 512 | ||||||||||||||||||
| else | ||||||||||||||||||
| const T8_BUFSIZ = 8192 | ||||||||||||||||||
| end | ||||||||||||||||||
|
Comment on lines
+261
to
+269
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the actual problem. t8code defines a The value of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. First of all: Thanks A LOT @benegee for identifying and fixing this issue! 👍 I can imagine this was not that much fun to find out 🙈 The CI seems to confirm you found the correct root of the crashing vtk output. That being said, I am not sure this is the correct way to fix it. Do you happen to have an idea why we overwrite the BUFSIZ like this in t8code? Seems like bad style to me, maybe we should replace it in t8code by some custom macro like And admittedly, I don't fully understand yet why the lines above fix the issue 🤔 because t8code seems to always set
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @spenke91 ! The displayed code is a bit misleading. This is just a local variable with "guessed" values for different platforms. I use it here Lines 274 to 280 in cf650db
t8_vtk_data_field_t structs, which should hopefully match the size in den underlying t8code library.
This is a fix because we used a hard-coded value of 8192 before T8code.jl/examples/t8_step5_element_data.jl Line 198 in 72031bc
In fact, if t8code would use a custom value instead of |
||||||||||||||||||
|
|
||||||||||||||||||
| # convenience constructor | ||||||||||||||||||
| # - adds 0 termination and padding to description string | ||||||||||||||||||
| # - takes pointer of data | ||||||||||||||||||
| function Libt8.t8_vtk_data_field_t(type, description::String, data) | ||||||||||||||||||
| return t8_vtk_data_field_t(type, | ||||||||||||||||||
| NTuple{T8_BUFSIZ, Cchar}(rpad(description * "\0", T8_BUFSIZ, | ||||||||||||||||||
| ' ')), | ||||||||||||||||||
| pointer(data)) | ||||||||||||||||||
| end | ||||||||||||||||||
|
|
||||||||||||||||||
| function t8_free(ptr) | ||||||||||||||||||
| Libt8.sc_free(t8_get_package_id(), ptr) | ||||||||||||||||||
| end | ||||||||||||||||||
|
|
||||||||||||||||||
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.
Manually changing Libt8.jl means that following the steps in dev/ produces another Libt8.jl, which is not ideal. Could we instead apply the fixes in dev/fixes.sh?
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.
Sorry @JoshuaLampert! Marking as "ready for review" was a mistake!
Yes, this has to go to fixes.sh, but I wanted to discuss first if there are better options.