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

Minor refactoring of weapons (beam/bolt) #561

Merged
merged 3 commits into from
Sep 27, 2021

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Sep 26, 2021

This PR is part of an ongoing refactoring of the code base. It is for the next release.

Please answer the following:

Code Changes:

Issues: None due to this PR. I will be opening multiple bugs later.

Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

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

This code seems functional enough, and it definitely fixes the "invisible beam" problem. However, I think we need to address the 4 "Multiplication result converted to larger type" issues before we merge this.

@royfalk
Copy link
Contributor Author

royfalk commented Sep 27, 2021

This code seems functional enough, and it definitely fixes the "invisible beam" problem. However, I think we need to address the 4 "Multiplication result converted to larger type" issues before we merge this.

I usually don't mind doing some minor edits of nearby code, but this ain't it. I moved these functions from beam_generic to beam without touching them at all. You can tell there are 4 "fixed" warnings in beam_generic.

It's tempting to cast each part of the expression to double just to get rid of the warning but I vote against. This is a part of the code I don't understand at all and the original developers had a fondness for pushing the envelope in terms of what can be done. My concern here is someone originally said "let's do this by having it overflow in float" and fixing it would introduce some subtle or not so subtle bug.

I am fine with a new issue to deal with this, assigned to me. I'll get to it this week and test it separately. I'll also try and understand the code a little and maybe polish some of it.

Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

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

OK, fair enough. Approving.

@royfalk royfalk merged commit 5e54110 into vegastrike:master Sep 27, 2021
@royfalk royfalk deleted the sep_21_more_unit branch September 27, 2021 19:31
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.

None yet

3 participants