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

Task component #596

Merged
merged 5 commits into from
Oct 30, 2021
Merged

Task component #596

merged 5 commits into from
Oct 30, 2021

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Oct 28, 2021

This PR is part of a refactoring of the unit class. Specifically, it introduces the concept of resource.
A resource is any part of the game that can be used up and filled up.
The purpose of this class is to simplify code throughout the game by placing it here.
Instead of adding to health and then checking it isn't above max health, it's done automatically.
Money, energy, shield, armor are all examples of resource.

Note to reviewer:
This is a partial implementation of resource as a template. If I missed an overloaded operator or broke the rule of 0 or n, focus on that.

Code Changes:

  • Have the PR Validation Tests been run?

Issues:

  • Can't really die. Tried twice and ejected automatically. Couldn't respawn manually using the ; key.
  • Milspec packages are broken. If you buy a ship with one, it will come with extra capabilities. You can't downgrade by adding crappy armor for example. However, if you upgrade to better armor and then sell, it will revert to 0.

Purpose:

  • What is this pull request trying to do? See above
  • What release is this for? Next one
  • Is there a project or milestone we should apply this to? No

A resource is any part of the game that can be used up and filled up.
The purpose of this class is to simplify code throughout the game by placing it here.
Instead of adding to health and then checking it isn't above max health, it's done automatically.
Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

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

LGTM - one minor comment, but not blocking.

engine/src/cmd/energetic.cpp Show resolved Hide resolved
@stephengtuggy
Copy link
Contributor

I've encountered the situation before where I eject instead of dying. However, we need to make sure that this doesn't happen 100% of the time, with these latest code changes.

@stephengtuggy
Copy link
Contributor

Also, unfortunately, some of the CI builds broke. :-/

@BenjamenMeyer
Copy link
Member

looks like Mac builds are breaking

@royfalk
Copy link
Contributor Author

royfalk commented Oct 29, 2021

looks like Mac builds are breaking

Fixed now. Note that I had a non-recurring seg-fault on one unit test. Since the test was also run successfully on other builds including other mac builds, I figured this was a fluke of some kind and re-ran the build and it passed.
However, it is something to keep in mind in general and when approving this PR.

@royfalk
Copy link
Contributor Author

royfalk commented Oct 29, 2021

I've encountered the situation before where I eject instead of dying. However, we need to make sure that this doesn't happen 100% of the time, with these latest code changes.

Opened #600 for it.

@royfalk royfalk merged commit 5336ba5 into vegastrike:master Oct 30, 2021
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