Modern Bgeo

[AlexeySmolenchuk]

Hi @davvid,
This isn't finished yet, but I want you to have a look at the proposed modifications.

The first time I touched partio several years ago I was confused by the incompatibility .bgeo reader with the modern Bgeo format.
Recently I discovered this Laika project: https://github.com/LaikaStudios/bgeo_reader and wanted to add JSON-based .bgeo support.

Regarding dependencies: It only links to the Houdini JSON parser and doesn't consume a license. https://www.sidefx.com/docs/hdk/_h_d_k__g_a__using.html#HDK_GA_FileFormat

Due to the ambiguity of '.bgeo' I moved the existing implementation to the BHCLASSIC.cpp and wrapped both in a chain of calls where .json version is first and .bhclassic is a fallback (I think in the production environment it's more common)

I only tested reading of .bgeo now, I'm going to explore writing soon.
It's all optional in CMake of course, but I think in the environment where Houdini is available it's a great advantage.

Please let me know what you think about this.

[davvid]

One of the tenets of partio is that it can read these formats without a Houdini dependency, or let alone any dependencies at all outside of the C++ stdlib.

I know that's not trivial by any stretch, but there might be some documentation about the file format that could be used to implement a standalone reader.

[davvid]

Sorry for not looking too closely, but if the only dependency is Houdini's json parser then I would instead recommend adding a dependency on rapidjson and using that neutral parser instead.

Dependencies are okay as long as they're FOSS.

[AlexeySmolenchuk]

Thank you @davvid.
What do you think about format plug-in architecture, this way it won't be Partio dependency anymore.

[davvid]

Adding a way to load DSO plugins would be useful in general.

Implementing it that way would also let us keep the original filenames and cmake setup, which should minimize the diff of the existing parts. This new feature would then be mostly additive changes in a new src/plugin/bgeo_reader.cpp file.

My sug would be to install the plugin to ${CMAKE_INSTALL_LIBDIR}/partio/bgeo_reader.so by default.

Implementing just enough to allow partio.load_plugin('<prefix>/lib/partio/bgeo_reader.so') would be a good minimal starting point. It'd be up to the user to run that step in their C++ or python code.

If we wanted to make it extra convenient, also providing partio.load_builtin_plugin('bgeo_reader') would be a nicer API. partio would handle knowing to look in its <prefix>/lib/partio directory and would handle appending the platform-specific .so suffix. That way the user only has to provide the plugin basename w/out any file extension in order to make it available.

cmake can forward its ${CMAKE_SHARED_LIBRARY_SUFFIX} for the .so part and it can also provide a PARTIO_PLUGIN_DIR #define for the code to use when implementing load_builtin_plugin(...).

The file format link does make it look like handling the text json format in the core would be relatively straightforward in the future. It's TBD whether the binary format can be easily reverse-engineered. Either way, it'd still be useful to have a bgeo_reader plugin both as a reference plugin and as a way to compare against some hypothetical future builtin reader.

Sorry for not getting back earlier but if you're interested in adjusting this MR to minimize diffs of existing lines and to rework this feature into a plugin then that'd be a fine way to integrate this IMO.

As for the plugins themselves, it seems like we'd have to expose these two typedefs and structs.

typedef ParticlesDataMutable* (*READER_FUNCTION)(const char*,const bool,std::ostream*);
typedef bool (*WRITER_FUNCTION)(const char*,const ParticlesData&,const bool,std::ostream*);

class PluginState {
public:
    PluginState(
        map<string, READER_FUNCTION>& readers,
        map<string, WRITER_FUNCTION>& writers
    ) : readers_{readers}, writers_{writers} { }
    void add_reader(const std::string& ext, READER_FUNCTION reader);
    void add_writer(const std::string& ext, WRITER_FUNCTION writer);
private:
    PluginState() = delete;
    map<string, READER_FUNCTION>& readers_;
    map<string, WRITER_FUNCTION>& writers_;
};

.. to the public API. Plugins would be expected to provide:

void initialize(partio::PluginState& plugin_state);

The core would invoke this function when loading a plugin during load_plugin().

The reason for the struct is to keep all of the state needed for initialization in one place. If we ever need to pass more in we can add it to PluginState. It'd also make it so that there's no way to construct a PluginState pointing to partio's internal maps outside of partio itself.

add_reader() and add_writer() would only act against the references handed to it during construction. It won't directly know how to dig into partio's internal maps. load_plugin() is where partio can construct PluginState instances by taking references to the internal maps returned by its private readers() and writers() functions. It would then hand a reference to the PluginState it constructed over to the plugin-provided initialize(...) function.

Just a rough sketch on one way that could work.

[AlexeySmolenchuk]

Thank you for your explanation.
Could mapping be like map<string, vector<READER_FUNCTION>>& readers ?
So multiple attempts to read files by different readers would be allowed.

[davvid]

That's a great idea. It might suggest that the return values from the reader + writer functions should be some kind of "Result" struct so that we can separate the, "this reader/writer cannot handle this file" scenario from the, "this reader/writer encountered an OS error" scenario.

Maybe they're not really worth separating and the READER/WRITER_FUNCTION interface is fine as-is? They already return a pointer for reading (meaning a reader can return nullptr for all error scenarios) and a bool for writing, so we could also just piggy back off of that and keep it simple for now until we need something more complex.

We can save the Result idea for later since we might not really ever need it. It could be useful for being able to pass error messages back to the caller, but we can always change that later since this plugin is going to be in-tree so we can change it in lockstep.

If we have a vector of readers + writers then the only other concern I can see is that the load_plugin(path) function might need a bool to indicate whether or not the plugin should be prepended to the list of plugins. We could default to false so that it appends by default. The bool load_plugins = false parameter would be the 2nd parameter for both load_plugin(path, prepend) and load_builtin_plugin(name, prepend).