zturtleman / mm3d

Maverick Model 3D is a 3D model editor and animator for games.
https://clover.moe/mm3d
GNU General Public License v2.0
115 stars 23 forks source link

Common functionality: mm3dcore or libmm3d? E.g. weld.cc, align.cc. #68

Closed m-7761 closed 5 years ago

m-7761 commented 5 years ago

I'm hesitant to add files, however there is both a Tool for extruding, and a Command for extruding, and I feel like neither is the proper home for the code that is common between them.

Needless to say, the way of the existing code is to duplicate functionality via copy-paste (bad programmer instincts) so how this breaks down in the case of extrude, is the window (UI) needs to implement extrusion, and so does the Tool, and the Command is actually just a short call to instantiate the window.

The "weld" tools have algorithms in the libmm3d folder. The "align" tools on the other hand have theirs in mm3dcore's.

I feel like it would better organized to make a single utility header for all of these, and implement the definitions in separate files if necessary.

The Tools and Commands all have their own files for the most part. The Commands especially are often very small files. I think it would be better to consolidate for the most part. And try to move as much code from the Tools/Commands/windows into the utility code, so that they become thin.

The Undo (technically ModelUndo for some reason) classes uncharacteristically are all implemented in a single file/header. They could easily have been 100 additional source files, but for some reason they ended up consolidated, even though I think they are more varied than the Tools and Commands.

Tools and Commands don't require headers for any reason. I've already removed those. But they still represent a lot of source files... they could be cobbled into one or two each, like the Undo files.

The Undo header would probably be better off removed, since it's not really required either, and I think it would make the 5000 line definition file easier to read and manage. Typically implementing short methods in the class body is easier to read than disembodied definitions. And it's definitely easier to have all the code in one stretch.

Anyway, I don't really understand the organization, however I think utilities of this nature belong in libmm3d, and not mm3dcore.

zturtleman commented 5 years ago

Yeah, probably add them to libmm3d since they are working with Model class.

m-7761 commented 5 years ago

EDITED: A minor, separate issue I see, is I feel like there should be a firewall between Command and UI code. Some of the commands just serve to open a modal window. I think it would probably be a better deal if the commands tried to work more like the tools, maybe by using a sidebar window to load widgets into like in the toolbar. That would permit the commands to be more versatile and tweak them in real time. (Blender does something like this. Though it's kind of confusing because doesn't have an Apply button to tell it you're satisfied/finished.)

I think the libmm3d utility code could try to hook up commands with suitable windows, however if there is going to be an actual plugin system, I think that the whole "script" idea might actually be a good way to build UI stuff, since it can be implemented in a such a way that the plugin doesn't interact with the concrete UI (assuming its worthwhile to flirt with having more than one UI implementation.)

I think there is probably a similar problem with translating (i18n) for plugins. My instinct is it would be better to find another way to do translation than Qt. I'm not a fan myself of scanning source code for message strings. And the Qt generated message catalogues have lots of duplication, that I think is rude to translators. It wastes their time. But technically I prefer to keep translation separate from code. And I think that it would be best to have a relatively neutral system, just as the UI can be kept apart.

In general the best way to manage translation IMO is to just build the reference catalogue side-by-side with the code. That is to say, don't try to automate it. And also I think it's best to put any disambiguation stuff into the ID strings themselves, and don't have a default translation. So that if something is missing from the catalogue it becomes obvious.