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

Use Vector types instead of tables for Scathis script #6528

Merged
merged 9 commits into from
Nov 18, 2024

Conversation

lL1l1
Copy link
Contributor

@lL1l1 lL1l1 commented Nov 10, 2024

Description of the proposed changes

Fixes #6524.

  • Changes the Scathis's script to use cached Vectors instead of local tables { x: number, y: number, z: number}
    This is more performant and easier to maintain.
    • For performance, the vectors are also accessed with indices instead of hash keys, avoiding a Vector's __newindex call.
  • Annotate and explain the Scathis script

Testing done on the proposed changes

There are 3 scenarios for the CreateProjectileAtMuzzle function changed in this PR:

  • Firing after unpacking (self.initialaim)
  • Firing while unpacked, but changing targets (self.losttarget)
  • Firing without changing targets (no special bool)

In all three scenarios the Scathis fires without errors in the log. The fake barrels are misaligned with the real barrel due to a refactoring error fixed in #6525.

Additional context

  • How should unit weapon annotations be handled? Classes seem to be global, so the name of the unit weapon class has to be unique. I opted for "[UnitID]_[WeaponName]" here.
  • I dislike how the script is designed because it interrupts the standard weapon cycle for its own animations, and would personally redesign it to use the OnStartTracking and OnStopTracking weapon functions.

Checklist

  • Changes are annotated, including comments where useful
  • Changes are documented in the changelog for the next game version

@lL1l1 lL1l1 marked this pull request as ready for review November 11, 2024 13:55
@Garanas
Copy link
Member

Garanas commented Nov 11, 2024

The use of tables as a replacement for Vector3(x,y,z) is not uncommon in the codebase. Do you anticipate more of these type of issues?

How should unit weapon annotations be handled? Classes seem to be global, so the name of the unit weapon class has to be unique. I opted for "[UnitID]_[WeaponName]" here.

I think that solution is fine.

I dislike how the script is designed because it interrupts the standard weapon cycle for its own animations, and would personally redesign it to use the OnStartTracking and OnStopTracking weapon functions.

I agree, and I also do not approve of the naming convention that is used 😄 . Feel free to pick this up in a separate pull request.

@lL1l1
Copy link
Contributor Author

lL1l1 commented Nov 12, 2024

The use of tables as a replacement for Vector3(x,y,z) is not uncommon in the codebase. Do you anticipate more of these type of issues?

As long as the tables are using arrays instead of hash tables the utility functions should work on them; the functions should be written using array accesses.

@lL1l1 lL1l1 added the area: sim Area that is affected by the Simulation of the Game label Nov 18, 2024
@lL1l1 lL1l1 merged commit 64e1f66 into FAForever:develop Nov 18, 2024
5 checks passed
@lL1l1 lL1l1 deleted the Fix/scathis-new-vector-processing branch November 18, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: sim Area that is affected by the Simulation of the Game type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: faf develop scathis does not fire
2 participants