Dumb rotation refactor

[Goober5000]

"Dumb rotation" is rotation of a submodel without having an associated subsystem. The original implementation by Bobboau was quite hackish, involving a redundant traversal of the submodel hierarchy independent of all the rest of the submodel code.

This PR merges dumb-rotation into the rest of the code, so that neither the traversal nor the rotation calculations are duplicated. It also introduces separate dumb-rotation data structures to hold the rotation information (which fills in for the absent subystems) so that the rotation doesn't have to be brute-forced each frame. Finally, it uses Swifty's model_instance code so that it is not necessary to call ship_model_start and ship_model_stop whenever the game wants to interact with the submodels.

(Note that, since dumb-rotation doesn't require a subsystem, it can be used for submodel rotation on models that are not ships.)

This PR incorporates some of the tweaked model_read code from #358. It is also a prerequisite for finally merging the look_at feature into the codebase in a non-hackish way, as look_at will use some of the helper functions in this PR. (The previously proposed look_at feature used a redundant brute-force traversal just like dumb_rotation did, making for two redundant traversals, each with their own redundant calculations.)

I've tested dumb-rotation in-game using this patch, and the rotating submodels render as you would expect. Curiously, collision detection doesn't seem to work. Even more curiously, collision detection didn't work on pre-refactor dumb-rotation either! I want to get collision detection working before this PR is merged, but I thought I'd create it anyway just to get additional eyes on it in the meantime.

[Goober5000]

Here's the mod I use for testing. It includes the standard Faustus, a Faustus X using dumb-rotation instead of standard rotation, and the Praetor ship, which uses look-at. (Ignore the Praetor for now; look-at will be re-implemented after dumb-rotation is finished.)
http://scp.indiegames.us/temp/praetor.zip

Sorry for the triple-post; github was derping out with attachments. I've uploaded it instead.

[asarium]

The changes look good. I haven't tested it but I'll do that as soon as possible.

model_do_dumb_rotations could be split into two separate functions, one that handles a single instance and one that handles all non-ship instances. I think that would make the code much clearer.

[asarium]

I tested the changes. Dumb rotations still work like before but there is no collision detection (but that's also the case in master).
I don't think no collision detection is a major issue because the current code also doesn't support it. If it is needed it can be added later.

[MageKing17]

I concur; if collision detection doesn't work right with it currently, then it not working with this PR isn't a regression.

[Goober5000]

Ok, fair enough.

As for the model_do_dumb_rotations, I specifically grouped the functionality together in the same function because the two cases are so similar. It is perhaps easier to follow in the modified version of the function, versus just the diff.

There are a couple additional cases I want to add or test before this is merged:

1. Activate dumb_rotation for non-ship models (currently this hasn't been added yet, but it should merely require adding the appropriate hook in the appropriate spot)
2. Warn when a submodel has both regular rotation and dumb_rotation

I plan to look at those this evening.

[asarium]

Depending on the argument for model_do_dumb_rotations a separate code path is taken. The two code paths are similar but still different enough to split the functionality into two functions (e.g. model_instance_do_dumb_rotations and model_do_non_ship_dumb_rotations).

[Goober5000]

The two code paths are handling exactly the same functionality, just in slightly different ways for slightly different circumstances. They belong in the same function. If they were separated into two different functions, the tight coupling would be de-emphasized and the risk for confusion and code drift would be increased.

[MageKing17]

...Except the two code paths aren't sharing any code in model_do_dumb_rotations(). If all calls to model_do_dumb_rotations() with a negative argument were replaced with calls to model_do_non_ship_dumb_rotations(), there would be very little difference.

[Goober5000]

They aren't sharing any code, but they are certainly sharing functionality. As the comment preceding the function explains, the dumb-rotation is being applied in two slightly different ways depending on the requirements of whether the thing is a ship or not a ship.

And the actual angle updating already was split out into model_do_dumb_rotations_sub because that code is exactly the same for both cases, and for that the separation makes sense. Another way of putting it is that it's appropriate to split the functions with an outer/inner separation, but not appropriate to split them along a lateral separation.

In other news, I figured out why collision detection wasn't working and added a commit to fix that (975e108). Along with adding the model_instance code for non-ship models (1651242) and making a few other fixes, that finishes up all the changes for dumb-rotation. Pending approval, this PR is ready to be merged.

[asarium]

Would moving the check (or saving the result in a bool variable and checking that) into the for-loop work for you? That would keep the code together and improve readability at the same time.

[Goober5000]

Which check do you propose to move into the for-loop?

[asarium]

model_instance_num >= 0

[Goober5000]

I don't think so. I tried hacking the code around a bit just now and it isn't as good of a solution. It would interleave the two code paths too much; the distinction between the two circumstances would be muddled. It actually makes it less readable because you have to puzzle out the two different execution flows.

[asarium]

If it doesn't work then I'd say the code can stay as it is. It's a minor style issue that doesn't cause any issues.

With that out of the way, I'd say this is ready to be merged.
@MageKing17: Any objections?

[MageKing17]

As noted earlier in the look_at PR, checking for both "$gun_rotation:" and "$gun_rotation" is redundant; might as well get rid of the former while we're at it.

[Goober5000]

Good point. Done, and I've removed colons from a couple other places as well.

[Goober5000]

@MageKing17 signed off on merging this, woot!

Next stop, fixing look_at. Next next stop, 3.7.4.