croft,
I have had a chance to look at your changes. They look great to me. I think your enhancements are very useful.
I have a few non-functionality related issues which may sound petty but I feel are important for a community project such as this one:
1. When making changes to code such as this, it’s better not to combine formatting changes with functionality changes, as the latter can get lost in the former (making it harder for someone evaluating to actually see what’s changed). I’m referring to changes like this:
- return new MD2RenderedFrame(frame.getName(),fanArray,stripArray,fanTexArray,stripTexArray,fanCounts,stripCounts);
+ return new MD2RenderedFrame(
+ frame.getName(),
+ fanArray,
+ stripArray,
+ fanTexArray,
+ stripTexArray,
+ fanCounts,
+ stripCounts);
}
By all means make formatting changes that help readability as they do in this example, but please keep those changes to a seperate commit.
2. Is there any reason the entire class is indented one level? Because every line is changed, diff will show the entire file being removed and replaced. While on my machine I can (and did) use the -w argument, the diff’s generated by Collabnet (which can be handy) will be useless.
3 Is there any reason why you havn’t followed the coding style of the original MD2Loader / xith3d project in general? We don’t put brackets on a new line, nor use the bulk slashes for comments. To me, MD2Loader is more readable than MD2Loader2, but stylistic preferences aside, I think it is always best to stick to the one style for a project, and at least stick to the same style for a single package-tool. If you were adding a stand-alone tool of your own creation, I wouldn’t mind, but having two different styles used in the one package I think makes it harder to understand the package as a whole.
To summarise: I think the API changes are excellent, but the formatting changes not so. Would it be possible to keep the code style as it is?
I really hope I havn’t offended you, the last thing I want to do is push away someone who is making a valuable contribution to Xith3D as you are - but I feel I need to raise these issues.
Best Regards,
Will.