-
Notifications
You must be signed in to change notification settings - Fork 13
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
Comments
Made some preliminary plans and it seems, a refactoring for If you are not opposed I'd work on a pull request just implementing the refactoring, but I'm always open to discussion. |
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. |
In theory I agree, but practically every exporter might define its own program options and use them accordingly. 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 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. Then you'd call 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. |
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. |
Apparently |
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:
The individual exporters'
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 An alternative without reflection could be to use interface for exporter options and
That interface would be implemented by Another alternative would be to pass I'll prepare something using the first approach. |
I've been using this codebase to write my i3d exporter, and in my opinion here is the architecture I would use:
or
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. 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. |
Save the data in the same way that the editor saves it
The text was updated successfully, but these errors were encountered: