Skip to content

Add additional safety, logging to CLuaBaseEntity bindings #6629

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

Merged
merged 1 commit into from
Jan 1, 2025

Conversation

claywar
Copy link
Contributor

@claywar claywar commented Jan 1, 2025

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Searches existing code in lua_baseentity.cpp for static_cast<CBattleEntity*> and audits existing code for appropriate checks, since NPCs can potentially call these functions and cause failures.

If NPC check is currently existing, code is not modified, but if no check exists, it converts to dynamic_cast. In some cases, types were modified to reflect the expected function (ex: CCharEntity* being passed into CBattleEntity* parameter)

Steps to test these changes

Check expected result from valid target(s), attempt to call function on NPC to see error messaging and no crash

@claywar claywar merged commit 97316d9 into base Jan 1, 2025
14 checks passed
@claywar claywar deleted the lua-baseentity-safety branch January 1, 2025 17:36
@claywar
Copy link
Contributor Author

claywar commented Jan 4, 2025

I'd say in this case it more exposes another issue now that logging exists. The following function expects CCharEntity, and we're stuffing some other type into it somewhere. Most likely the underlying battleutils function needs to be audited, along with our usage of it in general:

image

@claywar
Copy link
Contributor Author

claywar commented Jan 4, 2025

Correct since it will bail out of the function on that warning. Should be resolved by #6652

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants