Skip to content

Look_at reimplementation#530

Closed
Goober5000 wants to merge 13 commits into
scp-fs2open:masterfrom
Goober5000:look_at_reimplementation
Closed

Look_at reimplementation#530
Goober5000 wants to merge 13 commits into
scp-fs2open:masterfrom
Goober5000:look_at_reimplementation

Conversation

@Goober5000

Copy link
Copy Markdown
Contributor

NOTE: This isn't quite ready to be merged yet! See comments below.

This PR re-implements the look_at feature, which originally was a brute-force iteration down the submodel hierarchy, just like dumb_rotation. The look_at code now properly uses the model_instance code developed by Swifty to integrate into the rest of the collision and rendering code.

A look-at rotation is a type of intrinsic_rotation, just like dumb-rotations. The same code handles both; the only difference is whether submodel_rotate or submodel_look_at is called to apply the actual movement.

The previous look_at parsing code has been optimized by performing the name-to-submodel resolution when the model is actually parsed, instead of when the first look-at rotation is called. This avoids the need to store a name in the submodel struct and also avoids cluttering the look_at function.

A few functions - model_instance_find_obj_dir, find_submodel_instance_point, and find_submodel_instance_point_orient - have been tweaked to allow them to be called for any model with a model_instance, not just ship models.

This PR is essentially bookkeeping, except for the submodel_look_at method. Bobboau's previous code has a few flaws - it doesn't work with the new model_instance code, it only accounts for rotation in one level of the submodel hierarchy, it makes an unnecessary detour out to world coordinates - so it can't be used as-is. Unfortunately, it's also almost entirely inscrutable, with one- or two-letter variables and no comments. :-/ So it was necessary to rewrite the rotation calculation code.

Currently, the rotation code has a few bugs that need to be worked out before committing (assistance with the finer points of vector math is appreciated). The rotation has to work properly for all three axes, and it also has to work for all ranges of angles. Currently, only the X axis can be tested with the Praetor model, and currently the door struts flip 180 degrees in the process of opening.

The praetor test mod has been updated since the dumb-rotation PR. It can be downloaded here:
http://scp.indiegames.us/temp/praetor.zip

@Goober5000 Goober5000 added mantis A feature or issue imported from MANTIS, the old issue tracker cleanup A modification or rewrite of code to make it more understandable or easier to maintain. labels Feb 1, 2016
@Goober5000 Goober5000 added this to the Release 3.7.4 milestone Feb 1, 2016
@MageKing17 MageKing17 mentioned this pull request Feb 1, 2016
@asarium

asarium commented Feb 3, 2016

Copy link
Copy Markdown
Member

If possible, renaming dumb rotations to Intrinsic rotations should be done in a separate PR. Mixing the look_at reimplementation and the renaming makes code review very hard.

EDIT: I just realized that in your code Intrinsic and look_at rotations are actually the same thing. With that in mind it makes more sense to combine the changes but I would still prefer to have a separate PR which renames dumb rotations.

Comment thread code/model/modelread.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing the vector here isn't necessary since it will be deallocated anyway when the function finishes.

@asarium

asarium commented Feb 3, 2016

Copy link
Copy Markdown
Member

I added some comments but I haven't looked at the vector math yet. I'll do that at home when I can see the whole code in a real editor.

@Goober5000

Copy link
Copy Markdown
Contributor Author

PR #531 created, which is basically just the refactor of dumb_rotation to intrinsic_rotation. Once that PR is approved, I'll rebase this one.

@Goober5000 Goober5000 force-pushed the look_at_reimplementation branch from 0382b56 to 77f27a9 Compare February 5, 2016 23:34
@Goober5000

Copy link
Copy Markdown
Contributor Author

PR rebased, and I added a new commit which addressed most of the comments here. I'm reluctant to merge the model_num and model_instance_num parameters because there are about twelve function prototypes scattered throughout the code that use both.

I still need to fix the angle calculations, but I've heard back from Bobboau and I have another math guy willing to look at it, so the investigation is progressing.

@asarium

asarium commented Feb 6, 2016

Copy link
Copy Markdown
Member

I understand the concerns. Adding Assertion(model_num == pmi->model_num, "Instance model number does not match the specified model number!"); would catch any coding mistakes without changing the function signature.

@Goober5000

Copy link
Copy Markdown
Contributor Author

Again, if you're going to do that once, you might as well do it in all twelve functions. That's better suited to another PR.

I suppose I could make such a PR.

Goober5000 pushed a commit to Goober5000/fs2open.github.com that referenced this pull request Feb 6, 2016
@asarium

asarium commented Feb 6, 2016

Copy link
Copy Markdown
Member

That's even better! My suggestion was to just validate the instance and model number without changing the function signature but removing the parameter is the better solution.

@Goober5000

Copy link
Copy Markdown
Contributor Author

Yeah, I figured, if you want a PR anyway, might as well use the better solution.

The model instance code is fairly well debugged by now, but there is admittedly a possibility that the parameters could be transposed. This mitigates that risk.

Looks like there were some problems. I'll fix those. Once that PR is merged, I'll rebase this one again.

@Goober5000

Copy link
Copy Markdown
Contributor Author

I just figured out that the reason I'm having so much trouble with the vector math could be that the test model itself is set up incorrectly. :-/

In addition to preventing my current tests from working, this would have required Bobboau's original code to not only be inscrutable, but also use incorrect calculations.

I shall be in contact with a modeler shortly to deliver a test model according to my personal specifications.

@Goober5000

Copy link
Copy Markdown
Contributor Author

I've posted a model request:
http://www.hard-light.net/forums/index.php?topic=91507.0

asarium added a commit that referenced this pull request Feb 11, 2016
modify function headers as suggested in PR #530
@Goober5000 Goober5000 force-pushed the look_at_reimplementation branch from 5d1a19e to ecb9365 Compare February 11, 2016 20:02
@Goober5000 Goober5000 force-pushed the look_at_reimplementation branch from 8c1b333 to 91185dc Compare February 27, 2016 19:06
@The-E The-E modified the milestones: Release 3.7.4, Release 3.7.6 Mar 1, 2016
@Goober5000 Goober5000 force-pushed the look_at_reimplementation branch from ebfb8ff to 1c69bee Compare March 19, 2016 21:29
@Goober5000 Goober5000 force-pushed the look_at_reimplementation branch from 1c69bee to 8a16b80 Compare March 20, 2016 00:06
@MageKing17 MageKing17 added the unfinished (Deprecated by PR Drafts) A feature or issue that's unfinished, but still wants feedback/review label Apr 4, 2016
@TRBlount TRBlount removed this from the Release 3.8 milestone Jun 7, 2017
@Goober5000

Copy link
Copy Markdown
Contributor Author

Closing in favor of #3144.

@Goober5000 Goober5000 closed this Jan 14, 2021
@Goober5000 Goober5000 deleted the look_at_reimplementation branch January 14, 2021 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup A modification or rewrite of code to make it more understandable or easier to maintain. mantis A feature or issue imported from MANTIS, the old issue tracker unfinished (Deprecated by PR Drafts) A feature or issue that's unfinished, but still wants feedback/review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants