SC2Mapster / m3addon

Blender Addon to import and export m3 files
GNU General Public License v2.0
49 stars 18 forks source link

remove trailing spaces #26

Closed evie404 closed 4 years ago

evie404 commented 4 years ago

Hi, I'm interested in working on the plugin more, mainly to understand why animations are not loading beyond the first frame and attempt to fix it. In the process, I might do some cleanups, let me know if this is ok. If not, I can keep these changes to my own fork.

If you review this with whitespace diff suppressed: https://github.com/Talv/m3addon/pull/26/files?diff=split&w=1 you can see there's only whitespace-related changes.

Thank you for your time.

Talv commented 4 years ago

Hmm.. I'm not sure how I feel about this. I'm fully aware that there's a lot of inconsistency in regards to code formatting, beyond trailing white spaces, but I have chosen to let it be. And if anything fix it gradually. The reason behind that is that there's no short-term benefit in clearing that up. And the project is pretty much in maintenance mode right now. I've stopped any work on the plugin for the time being.

The other reason I was hesitant on merging it, was because it'd mess up the git blame which is super useful when tracking bugs on legacy code base such as this one. But I've just learned there's an option to ignore whitespaces, so I've no problem with this now.

I'm interested in working on the plugin more, mainly to understand why animations are not loading beyond the first frame

While this plugin has many problems, that one sounds rather odd. It's possibly a model specific problem. For instance, in Heroes of the Storm, model files of hero units (containing mesh, rigging, materials etc.) and animation files are separate. If that's the case you first need to import m3 and then corresponding m3a. And you also need to use other branch of this plugin, which is in #21.

evie404 commented 4 years ago

Thank you for merging this.

With regards to the animation problem, it ends up being related to the exact the issue you described with m3 vs m3a files. Do you know if material animation is supported by this plugin? That seems to be the next issue I'm running into.

Either way, I'm still interested in contributing to this project, even just for small quality-of-life improvements. For example, I added the functionality to prepend a prefix to object names when importing models, and grouping them in a collection. I find this to be more organized when importing multiple m3/m3a files into the same project. If you think those would be useful, I'd love to open PRs to upstream those changes.

With regards to code maintenance, I think there are some benefit in cleaning up code as standalone commits rather than bundled with other changes. One is it makes it clear from commit history which commits are meant as cleanups (ideally no functional changes) vs. which commits actually intend to introduce functional differences. It does screw up git blame, as you mentioned, but git now allows certain revisions to be ignored, which helps migrate styles without ruining git blame. Either way, I think it's generally a good practice to run some basic formatting/linting onto the codebase as it makes it more approachable to newcomers. If you think this is too heavy-handed, I don't mind keeping those changes in my fork.

Talv commented 4 years ago

Do you know if material animation is supported by this plugin?

It is, until the MODLv30 at least. However pretty much all animatable properties aren't reflected in Blender's viewport. Plugin contributes them as a set of custom properties.

Either way, I'm still interested in contributing to this project, even just for small quality-of-life improvements.

That's nice, but what's your use-case of this plugin? I'm asking because I've noticed various people use it for different purposes, such as:

And they're all interested in different features. Generally I've no problems with merging anything that doesn't break exporting functionality, since modding SC2 is the primary reason why this plugin was put together.

And I'm pretty sure it currently cannot export anything that isn't in the master collection. Also there are some major obstacles in importing multiple models (with their animations) into the same scene, due to how plugin currently handles the animations. It would likely require major refactor.

git now allows certain revisions to be ignored, which helps migrate styles without ruining git blame

Good to know, I'll try that someday.

evie404 commented 4 years ago

all animatable properties aren't reflected in Blender's viewport. Plugin contributes them as a set of custom properties.

that makes sense. so one would potentially link those custom properties as drivers of shader values in order for them to be reflected by in blender?

your use-case

i don't really have a strong use case right now other than just playing around models in blender. maybe one day i'll actually produce something more meaningful like a render or animation.

break exporting functionality

i have not really touched this part of the code. is there a workflow (or tests i can run... although i didnt see any) i can follow to ensure that's not broken?

obstacles in importing multiple models (with their animations) into the same scene, due to how plugin currently handles the animations

i think this is exactly what i ran into... it seems like the plugin attempts to find existing bones before creating them, so if multiple imports share the same bone name, there are some collisions. i might look into fixing it.

Good to know, I'll try that someday.

if you're interested, is there a particular flavor of python formatting you're more used to? i don't mind following that guide to format existing code before making more changes.

thank you for your time