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

Implementation of Planetary Statuses #690

Merged
merged 3 commits into from
Dec 18, 2023

Conversation

BottledByte
Copy link
Contributor

@BottledByte BottledByte commented Dec 16, 2023

Implements PlanetaryStatus and AppliedStatus classes. See GH-676.
Planetary Statuses definitions are loaded from JSON files, as per GH-666, applied statuses are stored in save games. PlanetaryStatuses are not used anywhere in the game and there are no statuses even defined.

This change breaks saves.
Removes "permanent planetary events" feature, which was used as statuses 😒 . Without replacement.
Removes a lot of tests.
Removes Climate change galactic event and mechanic.
Breaks balancing of spawning of planetary events - I will not fix that.

@BottledByte BottledByte force-pushed the rd/planetary-statuses branch 2 times, most recently from f8d631a to 4344e6f Compare December 16, 2023 20:12
@tuomount
Copy link
Owner

Could everything except that climatic change random event done with these planetary statuses? I don't understand why you removed planetary events that would do all those effects on planets. Reason why there is that planetary event attribute on planet is that player cannot cheat if bad effect is on planet by loading and trying again. If you would keep those events that would not break the spawning balance. After event has happened that could be removed and just use the planetary statuses.

Or Am I missing totally the point of this pull request?

@BottledByte
Copy link
Contributor Author

There was very messy in-code representation of planetary status and event. "Status" was represented via "persistent event", which was nonsense. I scrapped entire code for "persistent events", taking events for "good/bad planetary condition" with it.

I indeed could do it like you say, make an event apply a status to planet after event activation.
Allowing events to apply statuses is a valid design, but that's to be done in the future.

I didn't do so, because I refuse to add hacks just because of existing bad design.
It's the map generation that has to be altered to place statuses on planets at generation time (preventing said cheating). Such change will also allow order of magnitude greater extensibility in the future. And allow combining planetary statuses with planetary events.

Think about it like this:

  • Why abandoned palace could not be discovered (event) on a fertile planet (status)?
  • Less code == less work 😄

Or Am I missing totally the point of this pull request?

Maybe just a little bit 🙂 . It is about adding a feature while reducing technical debt.
I don't consider breakage of game balance as a reason to keep technical debt.

@tuomount
Copy link
Owner

I indeed could do it like you say, make an event apply a status to planet after event activation. Allowing events to apply statuses is a valid design, but that's to be done in the future.

So when this pull request is merged, need to do issue about it.

I didn't do so, because I refuse to add hacks just because of existing bad design. It's the map generation that has to be altered to place statuses on planets at generation time (preventing said cheating). Such change will also allow order of magnitude greater extensibility in the future. And allow combining planetary statuses with planetary events.

Think about it like this:

  • Why abandoned palace could not be discovered (event) on a fertile planet (status)?
  • Less code == less work 😄

Sounds good and would make sense.

Maybe just a little bit 🙂 . It is about adding a feature while reducing technical debt. I don't consider breakage of game balance as a reason to keep technical debt.

I am just worried some tries master and it does not work. I have usually done changes that add new features which cannot be done in few changes, in separate branch and then that is later merged to master.
By the way there is probably save game test that will fail after this changes. It can probably get fixed by first making change that will only write new save file and then implement the loading part. It can be also disable for a while and enable later when risk of changing save games is less.

Remove some presistent PlanetaryEvents that were statuses,
without replacement. Remove CLIMATE_CHANGE galactic event.
Remove some tests and modify others to accomodate for PlanetaryStatuses.
More flexible, with safety checks, one hundred lines less,
all of this while adding empty newlines for readability
@BottledByte
Copy link
Contributor Author

I am just worried some tries master and it does not work. I have usually done changes that add new features which cannot be done in few changes, in separate branch and then that is later merged to master.

It does work, just messes up balancing (hardly an issue in "nightly") and breaks saves (not an issue at all, see below).

By the way there is probably save game test that will fail after this changes. It can probably get fixed by first making change that will only write new save file and then implement the loading part. It can be also disable for a while and enable later when risk of changing save games is less.

  1. Your game save-loading system intentionally crashes on version change.
  2. There is no save migration support.
  3. master branch is development branch.
  4. In Readme is written that saves may break.

Only logical thing is to remove the game save-loading test altogether, then write a proper save-loading system which handles save migration. Which is out of scope of this PR. And finally, tests are irrelevant for the player.

@tuomount tuomount merged commit 1f27053 into tuomount:master Dec 18, 2023
@BottledByte BottledByte deleted the rd/planetary-statuses branch December 27, 2023 21:24
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