-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Task aug unit #796
Task aug unit #796
Conversation
closes vegastrike#751 Also add tests and a few additional methods to store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have to some instructions for testing this.
Aside from the linting issue this looks good; please fix that. I'll be happy to test it if given some guidance - I haven't had any ships with cloak yet.
engine/src/cmd/unit_generic.h
Outdated
return 1; | ||
} | ||
return ((float) cloaking) / 2147483647; | ||
cloak.Visible(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter raising an issue here since it does need to return a value or be refactored to be void.
Perhaps this should be return clock.Visible();
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to have to some instructions for testing this.
- Visual - switch to view 6. Hit c and watch the ship slowly cloak itself and decloak itself.
- Pick a fight and then cloak yourself and check the enemy doesn't shoot you. He may accidentally ram you...
- Buy a cloaking device. Sell a cloaking device. The whole upgrade issue has not been tested properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@royfalk where can I buy a cloaking device? I have yet to see one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenjamenMeyer Try buying a cloaking device at an Aera planet or base, maybe? I seem to recall someone saying at some point that Aera bases and planets tend to have cloaking devices, even if others don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephengtuggy thanks for the suggestion...I'll have to try that out
Add missing return. Other minor fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. This code is clean, well organized, well written, and seems quite straightforward. Well done, @royfalk .
I believe you also fixed the linter issue, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with @stephengtuggy so approving; will still try to test it out once I can play long enough to find somewhere to buy a clock device from
This is a near complete refactoring of the cloaking in the game.
This is also a move from refactoring using subclasses to fields.
Instead of energetic, we'll have reactor and capacitors.
Not sure why it is picking extra stuff for this PR. It should only have taken 8668683.
Code Changes:
Game Changes:
Issues: