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

Readd Meteor Filler #60

Merged
merged 24 commits into from
Nov 22, 2024
Merged

Readd Meteor Filler #60

merged 24 commits into from
Nov 22, 2024

Conversation

koolkrafter5
Copy link

Reimplement the concept of "filler" blocks to Blood Magic meteors.
Look for filler block list and fillerChance (chance for any individual block to be replaced with filler out of 100) in meteor config files in config/BloodMagic/meteors. Default filler block is minecraft:stone and default fillerChance is 0.
NEI meteor pages correctly show how much filler is expected in a meteor.
Fix an error in the block yield estimation function for more accurate results in NEI.

@koolkrafter5
Copy link
Author

Here is an example config file for a meteor that uses filler. Meteor is summoned with a 4:1 ratio of coal ore to iron ore, and 20% is replaced with sandstone as a filler block. Tenebrae, crystallos, and incendium may be added to replace the sandstone. Terrae/orbis terrae currently do not increase the filler chance.

CoalBlockMeteor.json

@AbdielKavash AbdielKavash self-requested a review November 16, 2024 05:47
Copy link
Member

@AbdielKavash AbdielKavash left a comment

Choose a reason for hiding this comment

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

Extra comments:

  • Add filler blocks to NEIMeteorRecipeHandler.loadCraftingRecipes, this means the meteors will show up when the player searches for recipes for e.g. stone. I can't see this ever being particularly useful, but it is consistent.
  • Not a huge fan of how the replacement filler (ice/obsidian/etc.) is currently generated in MeteorParadigm starting from around line 170. I know this is not your code. But since you now have a generic setMeteorBlock, you could precompute a new List<MeteorParadigmComponent> at the place where you check for reagents, and then just call setMeteorBlock on this list instead of fillerList if any reagents are present. (Or even better, replace sortedFiller by this list, so that it "magically" happens on its own.) This also future-proofs it for if/when we want to add more reagents.
  • Something to consider, and honestly I am not sure what is the best solution or even if this is a problem at all: Currently you have stone hardcoded as a default filler block, both in the NEI handler, and in the meteor spawning code.
    It would be perhaps more sensible to always initialize MeteorParadigm.fillerList; either in the constructor or when you are reading filler from config -- if fillerChance is not 0, and fillerList is empty, then add a stone block.
    Alternatively, MeteorParadigmComponent could have a static field that defines what the default block is?
    Since it is literally just stone I don't consider this a big "magic value" issue; but still if for whatever reason someone wants to change the default filler block, it is better to have it declared in one place.

@AbdielKavash AbdielKavash added enhancement ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version labels Nov 16, 2024
@koolkrafter5
Copy link
Author

koolkrafter5 commented Nov 16, 2024

Extra comments:

* [*]  Add filler blocks to `NEIMeteorRecipeHandler.loadCraftingRecipes`, this means the meteors will show up when the player searches for recipes for e.g. stone. I can't see this ever being particularly _useful_, but it is consistent.

Addressed in d9d70bb.

* [*]  Not a huge fan of how the replacement filler (ice/obsidian/etc.) is currently generated in `MeteorParadigm` starting from around line 170. I know this is not your code. But since you now have a generic `setMeteorBlock`, you could precompute a new `List<MeteorParadigmComponent>` at the place where you check for reagents, and then just call `setMeteorBlock` on this list instead of `fillerList` if any reagents are present. (Or even better, replace `sortedFiller` by this list, so that it "magically" happens on its own.) This also future-proofs it for if/when we want to add more reagents.

Addressed in 1af2bbb by making a new local fillerList and replacing it if any reagents are present.

* [*]  Something to consider, and honestly I am not sure what is the best solution or even if this is a problem at all: Currently you have `stone` hardcoded as a default filler block, both in the NEI handler, and in the meteor spawning code.
  It would be perhaps more sensible to _always_ initialize `MeteorParadigm.fillerList`; either in the constructor or when you are reading filler from config -- if `fillerChance` is not 0, and `fillerList` is empty, then add a stone block.
  Alternatively, `MeteorParadigmComponent` could have a `static` field that defines what the default block is?
  Since it is literally just stone I don't consider this a big "magic value" issue; but still if for whatever reason someone wants to change the default filler block, it is better to have it declared in one place.

Addressed in 3fe4b5b by setting the fillerList to stone with a weight of 1 when the weight is 0 or there is no block list supplied.

Minecraft crashes in MeteorParadigm.parseStringArray() if a filler or block list in a meteor config is written improperly. It appears to be a problem with parseInt in the legacy config section, and removing that section prevents that crash. Should there be more error checking there, or will that be removed soon enough that it won't be a problem?

It also crashes from a MalformedJsonException JsonSyntaxExpression in Meteor.loadConfig() if a meteor config is malformed (E.g. missing a comma). Should I move that to a try catch block add a catch block for that error and write an error to the log if that happens so Minecraft can continue to load?
Edit: Addressed in 4dc4573

@AbdielKavash AbdielKavash self-requested a review November 16, 2024 10:08
@AbdielKavash AbdielKavash removed their request for review November 16, 2024 10:11
…laced with filler.

Orbis terrae multiplies the filler chance by 1.12 (0.5 -> 0.56, filler chance is always truncated to the %).
Terrae multiplies the filler chance by 1.06 (0.5 -> 0.53, filler chance is always truncated to the %).
Meteors with 0% filler chance still have no filler.
In the old system, filler used to be calculated with the assumption that the max weight of every meteor was 1000, terrae would increase it to 1100, while orbis terrae would increase it to 1200.
If the total weight of the ores in the meteor was less than the new weight, all of it would be filled with filler.
If the total weight of ores was over 1000, certain ores would be locked behind (orbis) terrae.
There is (currently) no direct way to translate that functionality due to the radically different approach to handling filler.
Numbers were chosen based on how much filler would get added to meteors from the old system with about 600 total ore weight.
Exact numbers won't really matter because I intend to make reagents directly configurable in a config file, but that is outside of the scope of this PR.
Copy link
Member

@AbdielKavash AbdielKavash left a comment

Choose a reason for hiding this comment

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

Sorry about the delay; functionally everything looks good, check my comments for last couple of technical details.

Copy link
Member

@AbdielKavash AbdielKavash left a comment

Choose a reason for hiding this comment

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

All looks good now!

As I said above we will wait until 2.7 to actually merge this, so if you have any other ideas feel free to continue working on it.

@koolkrafter5
Copy link
Author

All looks good now!

As I said above we will wait until 2.7 to actually merge this, so if you have any other ideas feel free to continue working on it.

Awesome! I think I'll look into making the reagent effects for meteors configurable next.

@AbdielKavash
Copy link
Member

AbdielKavash commented Nov 21, 2024

All looks good now!
As I said above we will wait until 2.7 to actually merge this, so if you have any other ideas feel free to continue working on it.

Awesome! I think I'll look into making the reagent effects for meteors configurable next.

We should probably have a discussion about this before any big changes, because it might significantly affect balance. (Which might not be a bad thing necessarily.)

One thing I would like though is to keep the effect of the "base" reagents (as on the FTB wiki for example) unchanged, so as to not confuse people googling information about the rituals. There are plenty of other reagents we can use to add fun effects without having to change those.

@koolkrafter5
Copy link
Author

One thing I would like though is to keep the effect of the "base" reagents (as on the FTB wiki for example) unchanged...

That was my plan for when I would add this. The current reagents would stay the same in the default configuration, but it would leave the ability for people to tweak their effects and for other reagents to be added.

Latest commit was for something that simplifies the code added here and is useful for the reagent stuff, but I will make another PR from a different branch of my BM repo for the rest of the reagent configuration feature.

Could we possibly add dynamic pages to NEI that explain the reagent effects? I feel like that would make these and any future changes more discoverable for players.

@AbdielKavash
Copy link
Member

Could we possibly add dynamic pages to NEI that explain the reagent effects? I feel like that would make these and any future changes more discoverable for players.

That would be awesome, though ideally it should be done in a way that it works for all rituals.

@Dream-Master
Copy link
Member

Is this frozen or should be added for 2.7.0 ?

@AbdielKavash
Copy link
Member

This currently does not have any gameplay impact, as no meteors that are currently defined contain any filler blocks. Thus I think it is safer to delay until after 2.7. Then we can adjust the composition of various meteors, and possibly add filler where needed or reasonable. (This would be a balance change.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ongoing freeze - do not merge Not just a bug fix and thus affected by a current freeze for a upcoming version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants