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

Add .i3d XML exporting #23

Open
Donkie opened this issue Dec 14, 2021 · 7 comments
Open

Add .i3d XML exporting #23

Donkie opened this issue Dec 14, 2021 · 7 comments
Labels

Comments

@Donkie
Copy link
Owner

Donkie commented Dec 14, 2021

Save the data in the same way that the editor saves it

@thraidh
Copy link
Contributor

thraidh commented Aug 4, 2022

Made some preliminary plans and it seems, a refactoring for IExporter is needed. Today it is very much geared towards exporting single objs and the logic to convert the I3D class to multiple obj files is in Program.cs.
I'd propose to have an IExporter that would just take the I3D and probably the program options and the exporter would decide what it would write, where and how many files.
The main program would need an option to select the output format and based on that somehow produce the correct exporter class adhering to the IExporter interface. The "somehow" could be an ExporterFactory or just a simple switch in the main program.

If you are not opposed I'd work on a pull request just implementing the refactoring, but I'm always open to discussion.

@Donkie
Copy link
Owner Author

Donkie commented Aug 10, 2022

Hi, I agree the IExporter stuff is in dire need of some refactoring, I welcome any effort to improve that :)

I'm not so sure about your idea taking in program options into it though? Program options should be local only to the I3DShapesTool project and not exist in the .Lib project. The .Lib project should be seen as a separate library that others can use without the I3DShapesTool project, which is just the user interface.

@thraidh
Copy link
Contributor

thraidh commented Aug 10, 2022

In theory I agree, but practically every exporter might define its own program options and use them accordingly.
My idea would be that every exporter can define options, that are only relevant to it and consume them, without having the main program to care about what those options are.
Otherwise, an exporter would require an option and you'd have to edit the main program and the exporter and would have some way to pass the option values in an exporter-independent way. From my point of view that would be some tight coupling that should be avoided.

So my idea was, that every exporter is instantiated at the start of program, but they would be rather lightweight. They would define an options type and you'd basically call ParseArguments(args, optionTypes), where optionTypes is an array holding the types of CommonOptions and all the exporter-specific options. I'm not sure if that works, though.

Then you'd parse the input file just using common options, because that is always the same regardless of the used exporter.

There would be some new common option to select the exporter, which would be "OBJ" or so by default.
Somehow one of the exporters is selected, that can handle the selected exporter, maybe by iterating through the exporters and calling exporter.canHandle(selectedExporterTag) until one is found that matches.

Then you'd call selectedExporter.export(i3d, commonOptions, exporterOptions).
The export method might instantiate an object like InternalObjExporter, which would be almost the same as the current obj exporter, which would do the actual work, but how it is done is actually up to the exporter itself. The export method of a SimpleJsonExporter might just convert the whole I3D structure to json using some simple JSON serializer.

I haven't done a lot of C# development, so that may not even work the way I expect it to. Maybe I should just make some kind of POC, that you can comment on.

@Donkie
Copy link
Owner Author

Donkie commented Aug 10, 2022

The I3DShapesTool (the app) project being tightly coupled to the library is not an issue, it's by design tightly coupled to it. It's a simple and direct way for a user to interact with the library.

In my opinion, it should be up to each exporter how it wants to receive any options. Then we simply add support for whatever options is needed in the app by adding new command line arguments like it is already. If an option is clearly exporter-specific, we can prefix the option with the exporter name, for example "-obj:blabla 2". I don't think it will be that much bloat to handle all these specific options manually, there likely won't be that many of them. Regarding common options, that should be part of the IExporter interface somehow so that each exporter is forced to make use of them.

@thraidh
Copy link
Contributor

thraidh commented Aug 10, 2022

Apparently commandline doesn't work the way I thought it would, so I need to rethink.

@thraidh
Copy link
Contributor

thraidh commented Aug 13, 2022

I think I'm on a good path to understand your intention with the Lib and the App. Initially I thought it would be a good idea to let each exporter add command line options dynamically, but that a) isn't supported and b) would not be helpful, if someone wrote e.g. a UI which makes use of the Lib.

Different exporters still need different options and I'm a big fan of plugin architectures, i.e. you'd just add a new exporter implementation and everything is wired automatically, so I'll propose:

interface ICommonExporterOptions // contains all the common options
class ObjExporterOptions : ICommonExporterOptions // adds the OBJ-specific options
class I3DExporterOptions : ICommonExporterOptions // adds the i3d-specific options
...

interface IExporter {
   void export(I3D file, Action<ICommonExporterOptions> fillOptionsFunction);
   ...
}

The individual exporters' export method would create a corresponding IxxxExporterOptions object and run fillOptionsFunction on that.

I3DShapesTool would pass an action to export which would receive the options object of the exporter and fill it from CommandLineOptions using reflection.
That provides a nice decoupling and reduces the amount of places that need to be edited, when a new exporter is added.

The hypothetical UI tool would have to do something similar, but it might just use different actions for each exporter that fill the given options object using static code (i.e. something like void fillObjOptions(ICommonExporterOptions options) { options.Verbose = ui.getGeneralProgramOptionsAsBool("verbose"); ... }).

An alternative without reflection could be to use interface for exporter options and

interface ICombinedExporterOptions : IObjExporterOptions, II3DExporterOptions, ... {} // just combines all the exporter-specific interfaces

That interface would be implemented by CommandLineOptions and received by export instead of fillOptionsFunction. Each export would then narrow the combined options to its specific options interface.
Anyway, the only upside here is that there is no reflection, but that is bought with a lot more manual wiring, whenever a new exporter is added.

Another alternative would be to pass ICommonExporterOptions to export and downcast that in each exporter to its specific interface. This is probably the least effort, but downcasting is considered dirty and not type-safe.

I'll prepare something using the first approach.

@kbrandwijk
Copy link

kbrandwijk commented Sep 9, 2022

I've been using this codebase to write my i3d exporter, and in my opinion here is the architecture I would use:

  • Exporters always take the entire shapes file (so splitting of shapes into separate files would be an option for the OBJExporter, for example, and not a concern for the CLI tool)
  • Each Exporter exposes a set of options specific to that exporter, that can be used as command line options (for help output, and for command line parsing). A very common pattern in a lot of CLI tools (that I've used and developed) is to have commands that have their own unique set of options, so for example:
> i3dshapestool export:OBJ --splitShapes --generateSceneObjects

or

> i3dshapestool export:I3D --whatever

I use https://oclif.io/ a lot, where each command is a separate Class, and can have a base class for common options, so that's where I get most of my inspiration from on how to structure the CLI part of it. Of course, splitting lib and CLI here changes that a bit, but I don't think having the exporter classes provide the options that they can take is violating that separation.
It seems that CommandLineParser that is being used here also support loading these Verb classes, even dynamically through a plugin architecture (as in: each exporter is its own dll, drop it in the folder and it'll load the Verbs from it, and provide the exporter functionality).

I'd be happy to put up a proof of concept of that in a PR when I finish the exporter, unless y'all are strongly against that high level approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants