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

[Draft] Implement radar code as a separate component #804

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Oct 17, 2023

This PR introduces the concept of components - reactor, radar, cloaking device, etc.
This allows us to more elegantly move code from unit to components.
It will also allow for the removal of unit superclasses (e.g. movable) and easier upgrade, buy, sell, damage and fix components.

Let's use this to PR (and the cloaking one) discuss the concept of using components as a way forward.

Please answer the following:

Code Changes:

Issues:

  • None yet

Purpose:

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

double random20()
{
if(std::rand() < 0.2) {
return std::rand();
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be more like:

double r = std::rand();
if (r < 0.2) {
    return r;
}

the code you have will retrieve a random number, compare it, and if it's less than 0.2 retrieve another random number to return. not sure that's what was intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @BenjamenMeyer here.

@BenjamenMeyer
Copy link
Member

Yes, I like moving things to components. It makes a lot of sense.

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.

Nice! I'm excited about this new componentized approach. I think that this is what we've been needing for a long time. I look forward to seeing where it goes.

Some of the refactors I have in mind will also be considerably easier if Unit doesn't inherit from 50 million things LOL

double random20()
{
if(std::rand() < 0.2) {
return std::rand();
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @BenjamenMeyer here.

bool can_lock;
bool tracking_active;

std::unique_ptr<CRadar> original;
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh! This is interesting. Can you explain this a bit more, Roy? I'm interested to hear what it's for.

@@ -37,7 +37,7 @@ class Resource {
T adjusted_max_value_;
bool no_max_;
public:
Resource(const T &value, const T &min_value = 0, const T &max_value = -1);
Resource(const T &value = 0, const T &min_value = 0, const T &max_value = -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this constructor can be called with no arguments, or only one argument, it might be best to mark the constructor as explicit.

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