Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for glued cockpit #5631

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

Max5377
Copy link
Contributor

@Max5377 Max5377 commented Sep 18, 2023

Fixes #5621

This happened, because there was no check in this statement in void ShipCockpit::Update(const Player *player, float timeStep):
if (m_icc == nullptr),
if current CameraController is really InternalCameraController. Because of that, this statement in inline void ShipCockpit::resetInternalCameraController():
m_icc = static_cast<InternalCameraController *>(Pi::game->GetWorldView()->shipView->GetCameraController());,
would cast current CameraController to invalid type if it was not InternalCameraController, which is undefined behaviour.
Added check at the start of Update if current view is exterior to ensure that current CameraController is really InternalCameraController.

@sturnclaw
Copy link
Member

If there is now no codepath that calls resetInternalCameraController while the interior camera controller is inactive, the dynamic_cast should not be used. dynamic_cast has a significant performance overhead compared to static_cast and generally should be avoided in situations where the concrete type of a variable is guaranteed.

@Max5377
Copy link
Contributor Author

Max5377 commented Sep 27, 2023

Makes sense with a type-checking before casting. I will change this back to static_cast.
Also, maybe one more thing would be to put exterior view check at the top of Update, since player doesn't see cockpit in other cameras besides internal view, or your need to update rotation, etc. for cockpit even when player doesn't see it?

@sturnclaw
Copy link
Member

I have no intentions to further improve ShipCockpit.cpp. It is the last file in the codebase from the Paragon/Jumpdrive codebase (as indicated by the Meteoric Games copyright header), has many significant deficiencies/bugs, and is pending a complete replacement with a "clean-room" implementation of ship cockpit view that's being developed in my clickable cockpits branch.

You are welcome to make any changes to it that you deem necessary to improve performance or reduce unnecessary computation - however given that it will be completely gone in 1-2 releases I'm not going to ask you to spend any further time on it if you don't want to.

Were this another part of the code that isn't in "pending removal" state, I'd say go for it. 😄

Fixes issue when cockpit becomes "glued" on random occasions.
@Max5377 Max5377 force-pushed the GluedCockpitView-fix branch from 81951d4 to a128b00 Compare September 27, 2023 06:55
@Max5377
Copy link
Contributor Author

Max5377 commented Sep 27, 2023

Changed dynamic_cast back to static_cast and added IsExteriorView check at the start of Update.

@sturnclaw
Copy link
Member

Looks good! I'll test this in a few days in the same batch as your other PRs and (most likely) merge all three at once.

@impaktor
Copy link
Member

impaktor commented Sep 27, 2023

@Max5377 Please let us know if you want to be in AUTHORS.txt (and what to put there). Also: good work with the bug fixes!

@Max5377
Copy link
Contributor Author

Max5377 commented Sep 27, 2023

@impaktor Yes, you can add me in AUTHORS.txt. I write later what to add here, need to think about it. You are welcome.

@sturnclaw sturnclaw merged commit 90f7a0a into pioneerspacesim:master Oct 6, 2023
4 checks passed
@Max5377 Max5377 deleted the GluedCockpitView-fix branch October 8, 2023 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cockpit model occasionally becomes glued to the view at a random angle
4 participants