Loading of data collections #332
Conversation
|
@justinlaughlin , can you check if |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to load data collections by replacing StreamState with a more unified DataState and updating the file and collection readers accordingly. Key changes include:
- Refactoring threading, stream, and file reading modules to use DataState instead of StreamState.
- Updating collection reading and JS binding logic to be consistent with the new DataState API.
- Cleaning up legacy or unused functions (e.g. removal of Set_Texture_Image) for improved maintainability.
Reviewed Changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/threads.hpp & .cpp | Replaced StreamState with DataState and updated related API calls |
| lib/stream_reader.hpp | Removed StreamState methods; introduced a StreamReader class to work with DataState |
| lib/file_reader.[hp | cpp] |
| lib/coll_reader.[hp | cpp] |
| lib/data_state.hpp | New unified data state structure replacing StreamState |
| lib/aux_vis.hpp & aux_js.cpp | Updated auxiliary and JavaScript interop functions to work with DataState |
Files not reviewed (3)
- CHANGELOG: Language not supported
- lib/CMakeLists.txt: Language not supported
- makefile: Language not supported
Comments suppressed due to low confidence (1)
lib/file_reader.hpp:22
- [nitpick] The variable 'pad_digits' could be renamed to something more descriptive (e.g., 'file_pad_digits') to clarify its role in formatting file names.
int pad_digits;
|
That's a really useful capability, thanks @najlkin ! |
I'll check it out Edit: It built at least :) will see how it runs https://github.com/GLVis/glvis-js/tree/ci-build-14650298946 Edit 2: Tested basic functionality in |
|
Do you have any files that would be good as test cases? Maybe we can upload to |
|
Just run some of the examples and try to load the output data collections😉 . |
|
I still think it'd be good to upload some small sample data.
|
|
This looks very good though! Much cleaner implementation and it is nice to see so many lines of deleted code :) |
There was a problem hiding this comment.
This is a really nice addition, thank you @najlkin. 🚀
I agree with @justinlaughlin that it may be good to add some data collections samples (e.g. the same data from Example 5 or something in different formats) to the mfem/data repo.
| lib/palettes.cpp lib/palettes_base.cpp lib/sdl.cpp lib/sdl_helper.cpp lib/sdl_main.cpp \ | ||
| lib/coll_reader.cpp lib/data_state.cpp lib/file_reader.cpp lib/font.cpp \ | ||
| lib/gl2ps.c lib/gltf.cpp lib/material.cpp lib/openglvis.cpp lib/palettes.cpp \ | ||
| lib/palettes_base.cpp lib/sdl.cpp lib/sdl_helper.cpp lib/sdl_main.cpp \ |
There was a problem hiding this comment.
heads-up that there are changes here in the release branch https://github.com/GLVis/glvis/pull/331/files#diff-beda42571c095172ab63437d050612a571d0d9ddd3ad4f2aecbce907a9b7e3d0
There was a problem hiding this comment.
I see, the order is the same, right (?). We need to redecorate it after merging this and updating the release branch 🪄 .
Added some VisIt test files in mfem/data#11 😉 . |
This PR introduces loading of data collections following #325 and refactors the codebase. All collections with implemented readers in MFEM are supported, i.e., VisIt, Sidre, FMS, Conduit. The path to the collection is specified by
-visit/-sidre/-fms/-conduitparameter and the name of the field through-gor q-field through-q. Optionally, the cycle and protocol can be specified throughdc-cycleanddc-protrespectively.Alternatively, scripts can be used for loading. The commands are
data_coll_mesh/field/quad, where the first parameter is the integer corresponding to the type of the collection matchingDataCollectionReader::CollType. The cycle and protocol can be preset bydata_coll_cycle/protocol.Corresponding website update: GLVis/web#18