Killed the last vestiges of ship_model_start/stop.

[SamuelCho]

My hopefully last stab on bringing the rest of the code to the polymodel_instance system. This should make us no longer reliant on ship_model_start and ship_model_stop. What do you think @Goober5000?

[Goober5000]

Excellent! Preliminary review looks good. I will review it more thoroughly tomorrow. Perhaps you can clarify a few things in the meantime:

• Are the model_find_world_dir and model_find_world_point functions used anywhere else? Can they be removed in favor of their _instance_ variants?
• What's the purpose of render_info.set_object_number(OBJ_INDEX(objp));? (looks like it has to do with the rendering code)
• Why add submodel_instance_info *sii; to the submodel_instance struct? It already appears in bsp_info and twice in ship_subsys.
• Is it still necessary to call model_clear_submodel_instances() every time you call ship_model_update_instance()?

Also, is there anything that ship_model_start does that needs to be mirrored elsewhere? A brief look shows some texture resets, sm->num_arcs = 0;, and interp_clear_instance(), all via model_clear_instance().

[SamuelCho]

I think later down the line I'm going to streamline these model_find_ functions. I think eventually I'd rather have model_find_world_dir, model_find_world_point, and other similar functions optionally accept model_instance_num to lessen confusion instead of having separate model_instance_find functions. Maybe eventually we use the world transform we precompute in ship_model_update_instances() so we can actually start saving cycles.

bsp_info's sii can't be relied on. sii in bsp_info was tied to the instance that was set by ship_model_start. That's no longer valid since ship_model_start is going to be deprecated so we have to use what's in polymodel_instance.

Yeah, we may no longer have to call model_clear_submodel_instances() each frame. I'm not too sure though.

I'll double check to see what's in ship_model_start that needs to be carried over into the polymodel_instance system.

[SamuelCho]

Did a minor change to make sure some of the behavior we needed from ship_model_start in ship_render is carried over. Namely num_arcs and gun_submodel_rotation being zeroed out along with pm->maps[i].ResetToOriginal() being called.

[Goober5000]

Sorry for the delay in getting back to this. Last week was super busy.

I see that ship_model_start is still present in ship_render_DEPRECATED; is that still necessary there? The other uses of ship_model_start are either commented out or disabled. (Incidentally, if this PR really does kill ship_model_start/ship_model_stop then an additional commit should be made that removes the functions.)

Following your comment, I looked at the uses of bsp_info's sii in your branch and it is no longer accessed anywhere, just written. A commit should be made that removes this field as well.

Still curious about my earlier question - "Why add submodel_instance_info *sii; to the submodel_instance struct?" You answered about sii in bsp_info but that only answers half the question, as far as I can tell. (And you don't mention how that is different from the two fields in ship_subsys, submodel_info_1 and submodel_info_2, which are used in various places.)

I created a build based on this PR branch and loaded up a mission to test. Almost everything looked good -- submodels rotated properly, and rotating collisions worked. However, when I destroyed the Faustus solar panel, the panel simply disappeared instead of being replaced with its damaged version. Ditto with an Orion turret disappearing.

Once these are addressed, I'll give this PR one last test and then approve the merge. I think we can safely defer the model_find_world_dir, model_find_world_point, and ship_model_update_instance[s] changes to a future PR.

[MageKing17]

AFAIK, none of the DEPRECATED rendering functions are called and should probably be removed outright.

[SamuelCho]

sii is a reference to a ship subsystem in case other places in the code need it. If you want to see it's relationship to submodel_info_1 and submodel_info_2, look at ship_update_model_instance. Forgive me if I sound annoyed but I feel as though this should have been obvious looking at how those two identifiers were used in that function.

MageKing is correct about why ship_model_start was not removed in the deprecated ship render function. However, I would not recommend removing it until we're comfortable with the new rendering code.

[SamuelCho]

BTW I won't be making any additional commits to this pull request until the week after. I'm away from home and only have my work laptop. It's not a very good dev environment to do any bug repros or iteration fixes.

But in the meantime, can someone verify if those submodels are still disappearing in a build without the code changes in this pull request?

[Goober5000]

Well. >.< Now that Christmas and New Year's are out of the way, maybe we can close this out. :) @SamuelCho, by "the week after" were you missing a date (e.g. Christmas or New Year's) or did you mean the week after you posted that?

Regarding sii, the key piece of information I was missing was "a reference to a ship subsystem in case other places in the code need it". That makes more sense now. If I understand you correctly, it's for keeping track of the info in the intermediate functions that handle the rotation, while in the midst of a rotation.

However, given that clarification, I suggest adding a sii_2 field for submodels that use submodel_info_2. This is because the two model_update_instance calls in ship_model_update_instance cause submodel_info_1's sii to be overwritten by submodel_info_2's sii. This will cause problems in calculate_ship_ship_collision_physics, in the block at line 612 in your branch, where the code assumes that it is operating on the child submodel, not the grandchild. (It is probably not necessary to enhance the calculate_ship_ship_collision_physics code to take the grandchild rotation into account. All that is necessary is to avoid overwriting sii with the reference that belongs in sii_2.)

I concur with your recommendation against removing the deprecated functions until we're comfortable with the new rendering code.

Per your most recent note, I compared a build using the latest master against a build using your branch. In master, the submodels were properly replaced with their -destroyed variants. In your branch, the submodels simply vanish.

[MageKing17]

I don't really see any point in keeping the deprecated rendering functions. If we need them in the future, they'll still be there; that's the whole point of version control. Still, I suppose it is a somewhat bigger part of the codebase than some commented-out do-nothing functions...

Per your most recent note, I compared a build using the latest master against a build using your branch. In master, the submodels were properly replaced with their -destroyed variants. In your branch, the submodels simply vanish.

Did you test the branch by itself, or did you merge it with master first? Because the kill_ship_model_start branch doesn't have commit 0062d25.

[Goober5000]

Did you test the branch by itself, or did you merge it with master first? Because the kill_ship_model_start branch doesn't have commit 0062d25.

Bleh. I tested the branch by itself. Today I used git cherry-pick to add in commit 0062d25 (which is probably not the proper procedure, but it worked) and that fixed the vanishing submodel issue. Both rendering and collision switched to the -destroyed submodel.

[SamuelCho]

I don't think we need a separate sii_2. ship_model_update_instance already assigns psub->subobj_num with &pss->submodel_info_1 and psub->turret_gun_sobj with &pss->submodel_info_2. They're two separate submodels so I don't think any collisions are occurring. If they were occurring, we would've seen this when we were still using ship_model_start. ship_model_start does the same thing.

[Goober5000]

I stand corrected. Looking at the code more closely, there are indeed two submodel_instance fields that each have their own sii field, using the submodel numbers you mentioned, not just one. Sorry for the mistake. Along with the disappearing submodel fix from 0062d25, I believe that satisfies all implementation requirements for this PR.

Two last questions, though. First, at what point will we be "comfortable" with the new rendering code? I think we should set a date or a criterion for when the deprecated functions will be removed, because otherwise we have a lot of unused complicated code hanging around and making things confusing. After the release of 3.7.4, perhaps?

Second, why did you add those two new commits where you merged master into your branch? They make the PR diff significantly more complicated.

[chief1983]

Probably so that anyone checking out his branch has the latest master merged in? I would have recommended a rebase though, but ultimately reading the diff against master isn't that hard, via the github Files Changed tab above or manually diffing against master on a local machine. But there are still only three relevant commits in his branch, which I assume is correct.

[SamuelCho]

Yeah, chief is correct as to why I added those two commits. I can close and resubmit this pull request if you want a cleaner pull.

[chief1983]

Shouldn't be a need for that, if anything you can always force push over the same branch after rebasing on your machine (git push -f).

[Goober5000]

Yeah, I did all my code review on the initial commits before master was merged in. I like having a clean PR, but I'm not a stickler for it. Given that I already reviewed the earlier commits and we've been working on this for several weeks, I won't ask you to close and resubmit it.

In lieu of a response to the first question I'll assume that 3.7.4 is a good milestone to remove the deprecated functions.

With that, I'll merge this and then get to work merging in the updated dumb_rotation code and the revised look_at feature.