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

Bugfix/lab upgrading disallow researching #443

Merged

Conversation

rautamik
Copy link
Contributor

@rautamik rautamik commented Nov 10, 2024

Bugfix to #181

Changes:

  • Refactor object building requirements check. Split object requirements met check to different functions: base, with previous levels and with items in queues.
  • Disallow research when Research Lab is upgrading.

TODO;

  • check Research Lab upgrading from all planets, not only from current planet
  • disallow Research Lab upgrading when research is in progress.
  • disable all Research Lab upgrade buttons when research is in progess
  • fix multiplanet research cancellation bug

@rautamik
Copy link
Contributor Author

Hi @lanedirt,

Please check this object requirement check refactoring and fix to research lab upgrading PR when you have time.

Both cases, researching and research lab upgrading, should be covered and I have also included feature tests for both cases.

@lanedirt
Copy link
Owner

Hi @rautamik,

Thanks for your work on this! I just tested it on my local environment and looks very good overall, but I did notice two small things, see below. I also glanced over the code and it looks good to me. 👍 Also major thanks for taking the time to add the feature tests, that will surely help out with future changes or refactoring if needed!

Here are the two things I noticed:

  1. Resarch lab buttons are not all disabled
    When research is being carried out, the research lab shows the "Research is being carried out" which is good. However the upgrade button is still active for the "quick upgrade" and the normal upgrade buttons. I would expect these two buttons to be disabled as well:
Screenshot 2024-11-18 at 21 33 19
  1. Trying to build research lab results in research itself getting canceled
    An additional strange thing I encountered after playing with it a bit is the following:
  1. Start research on planet A
  2. Switch to planet B
  3. Attempt to upgrade the research lab. The research lab itself is grayed out but because the "build" button still being pressable (related to first issue mentioned above). When I press the build button nothing happens, i.e. the research lab doesn't get added to the queue which is good. However what I noticed is that after this action the actual research that was in progress is also canceled. I would expect the research to stay in the queue and not be affected. Note: this only happened to me when I try to build the research lab on a different planet. If I do both things on the same planet this issue doesn't happen.

@rautamik
Copy link
Contributor Author

Hi @lanedirt,

Thanks for testing and providing feedback.

I should have noticed that Improve button needs to be disabled, I have now committed changes to disable it. I didn't even know about the quick upgrade button, thanks for pointing out this. I have also fixed this issue.

Regarding the second issue, canceling the research, I was not able to reproduce it when the Improve button was still visible. Like you said, nothing happens to the Research Lab when it's tried to upgrade during the research, and that is because there is sanity check when build queue item is started. I was trying it by researching Energy Technology level 5 in planet A and trying to upgrade Research Lab level 3 to level 4 in planet B. Research should get cancelled only if requirements are missing so I wonder if there is something like it happens when specific level of Research Lab is being cancelled or something like that. In any case it should be possible to happen when players cannot start upgrading, when all buttons are properly disabled. I wonder if we could skip this issue, what you think?

@lanedirt
Copy link
Owner

Hi @lanedirt,

Thanks for testing and providing feedback.

I should have noticed that Improve button needs to be disabled, I have now committed changes to disable it. I didn't even know about the quick upgrade button, thanks for pointing out this. I have also fixed this issue.

Regarding the second issue, canceling the research, I was not able to reproduce it when the Improve button was still visible. Like you said, nothing happens to the Research Lab when it's tried to upgrade during the research, and that is because there is sanity check when build queue item is started. I was trying it by researching Energy Technology level 5 in planet A and trying to upgrade Research Lab level 3 to level 4 in planet B. Research should get cancelled only if requirements are missing so I wonder if there is something like it happens when specific level of Research Lab is being cancelled or something like that. In any case it should be possible to happen when players cannot start upgrading, when all buttons are properly disabled. I wonder if we could skip this issue, what you think?

Hi @rautamik,

Thanks for the fixes! I'll try to test the new version again later today.

Regarding the issue: ah yes I guess the level of the research lab might be the cause then. My planet B was empty, so when I tried to upgrade the research lab it was from level 0 to level 1. And after canceling the upgrade, the research lab on that planet is too low for the research in the queue (because level 0) so it cancels the research itself too.

I agree that if the upgrade button now disabled fixes this problem we can leave it at this. I'll double check this on my end in my tests later. 👍

Thanks again for your work 😄 !

Copy link
Owner

@lanedirt lanedirt left a comment

Choose a reason for hiding this comment

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

LGTM! Just tested the functionality again and works good for me in all usecases I could come up with. Thanks, will merge!

@lanedirt lanedirt merged commit ad69e30 into lanedirt:main Nov 22, 2024
6 checks passed
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