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

Rewrite/translation task #684

Merged
merged 14 commits into from
Aug 20, 2024
Merged

Conversation

Intybyte
Copy link
Contributor

@Intybyte Intybyte commented Aug 7, 2024

Describe in detail what your pull request accomplishes

Partial rewrite of the class

Checklist

  • Tested

Copy link
Contributor

@TylerS1066 TylerS1066 left a comment

Choose a reason for hiding this comment

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

While I appreciate the work and improvement to the codebase this PR (and it's twin) brings, I would like to avoid PRs that refactor classes without any performance or feature improvements (especially for classes that will need to be rewritten in the near future).

@TylerS1066
Copy link
Contributor

Another note, with the two recent PRs about collision explosions, you should bring that into this PR.

@TylerS1066
Copy link
Contributor

TylerS1066 commented Aug 18, 2024

Not fully sure why, but torpedos appear to just vanish when testing this.

EDIT: It appears that they are either being handled with gravity or max height and fall.

@Intybyte
Copy link
Contributor Author

Not fully sure why, but torpedos appear to just vanish when testing this.

EDIT: It appears that they are either being handled with gravity or max height and fall.

This was after or before the merge?

@TylerS1066
Copy link
Contributor

This was after or before the merge?

It was using the result of this actions build, triggered from the latest commit in this PR.

@TylerS1066 TylerS1066 merged commit 5246f5f into APDevTeam:main Aug 20, 2024
1 check passed
@Intybyte Intybyte deleted the rewrite/translationTask branch August 25, 2024 19:17
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.

2 participants