Ehamloptiran merge

[josephcrowell]

Please merge this to a new branch. All files are currently overwritten with Ehamloptiran's versions in their correct positions. This will make viewing with a diff tool much easier.

[josephcrowell]

The first thing I notice is that the whitespace is different.

[neomonkeus]

Adding @niftools/3ds-max-reviewers for review.

Code content wise, it is pretty unwieldy to review the whole thing. Not sure what is the best way to review this. Github only allows a limit on the visual diff, so probably need to do this locally for any of the files it won't display.

Here are the raw versions - https://github.com/smokex/max_nif_plugin/commit/9d72d9750cddae9cf922110039a20f282ff36717.patch
https://github.com/smokex/max_nif_plugin/commit/96a83fa62ea3dc07b2f469eb07a4011d8e7ae7b5.patch

One thing I did notice is that there are some file which Ehamp as the name -> EphamReadme for example. and the vcproj too.

[josephcrowell]

Yeah those files can't be merged as is but could have some custom build settings that he used.

[josephcrowell]

In regards to the code formatting, neo, do you guys usually follow any rules on whitespace?

[neomonkeus]

I suppose general coding conventions. If you mean ignore whitespaces on commit, then yes if possible as it makes it harder to review if nothing else.

[josephcrowell]

I'm talking about things like do you use 4 or 8 spaces for tab. It looks like Ehamloptiran used a code beautifier and corrected the tabs to 8 spaces while the original had 4. It also changed the spacing in the function arguments. This makes it look like identical code is different and that makes it needlessly hard to review. Should I change the tab spacing to 4 characters and make another commit?

Since the original files have spacing, removing all spacing in the commit would just make the issue slightly different but wouldn't solve it.

For example:

-   rKey.time = TimeToFrame(time + key.time);
-   rKey.flags = 0;
-   return rKey;
+   rKey.time = TimeToFrame( time + key.time );
+   rKey.flags = 0;
+
+   return rKey;

These lines are exactly the same code with different whitespace and these files are filled with that kind of mess.

[hexabits]

You can ignore whitespace during DIFFs so that it makes it easier for you to review.

[neomonkeus]

I personally don't mind what convention is used. However, I would suggest using the older one, to perform a functional review first. Then if the code style is preferable (and repeatable, not sure what environment you guys are using) we can always do a "formatting" commit.

[josephcrowell]

You can ignore whitespace during DIFFs so that it makes it easier for you to review.

Good to know. :) So it can be done in the gitlab diff interface?

[neomonkeus]

See - https://github.com/blog/967-github-secrets
Also, github vs gitlab :D

[josephcrowell]

Oh, oops I meant github. ;)

[blabbatheorange]

I wouldn't suggest it, Ehamloptiran broke a lot of base compatibility for older games, as well as made some few erroneous changes, as well as completely overhauled the build system. Also, no public version of niflibs will allow you to compile the plugin as it references collision methods that don't exist.

I'd heavily suggest looking at his changes and merging them into the existing plugin codebase and uploading that as a new branch for development of Autodesk plugins 2013+

[neomonkeus]

@blabbatheorange I agree. If I get time I will create a duplicate develop branch, and this can be merged and I can rename that branch to the proposed name.

Still it would be good for people to look through this all the same. Lets take the useful bits