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

Improve default enchantment API #1698

Merged
merged 5 commits into from
Apr 30, 2023
Merged

Improve default enchantment API #1698

merged 5 commits into from
Apr 30, 2023

Conversation

bruberu
Copy link
Member

@bruberu bruberu commented Apr 11, 2023

What

This PR is intended to fix an issue with tools being applied default enchantments from other tool types, as well as to expand the API slightly in the process, by adding functionality for letting default enchantment levels increase as the material level increases. The first issue had been made apparent by other sword items taking on the butchery knife's looting III.

Implementation Details

There is probably a better way to implement the default enchantment level increase, as well as a better way to name it in the code.

Outcome

Fixes other GT swords having Looting III unnecessarily

Potential Compatibility Issues

The new method added to IGTToolDefinition will require any other implementations of it to error, and anything reliant on the current oddity of GT default enchantments will also break as a result of this PR.

Copy link
Contributor

@Tictim Tictim left a comment

Choose a reason for hiding this comment

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

The whole ToolProperty class is unguarded against other modification factors, despite them using builder pattern for instantiation. Changing material stats mid gameplay aren't the best thing to allow so we should come up with some solution. Currently the setters are only being used in CT integration stuff, which I think there's a better approach than allowing modification absolutely everywhere.

For the default enchant growth API, I think approaches like this is better:

public ToolDefinitionBuilder defaultEnchantment(Enchantment e, int level){
  return defaultEnchantment(e, level, 0); // default to growth level of 0 per harvest level
}

public ToolDefinitionBuilder defaultEnchantment(Enchantment e, int baseLevel, int levelGrowth){
  this.defaultEnchantments.put(e, new EnchantmentLevel(baseLevel, levelGrowth)); // a new data type to hold base level and growth at the same place
}

Then the enchant level for items can be calculated like this.

// EnchantmentLevel.java
public int getLevel(int harvestLevel){
  return this.baseLevel + (this.levelGrowth*harvestLevel);
}

If we want to support partial growth rate (e.g. enchant level going up every 2 harvest levels), then maybe we could make base level and level growth as doubles. On the evaluation then we can calculate the level as usual, and ditch the decimal parts. But changing ToolDefinitionBuilder#defaultEnchantment argument to double is binary break, so we should make yet another method if we want to do this.

This change will require changing the return type of the IGTToolDefinition#getDefaultEnchantments, but I think it's a fair change because not factoring the growth level in is incorrect anyways.

I guess this API change can be also applied to ToolProperty#addEnchantmentForTools.

@bruberu bruberu added the type: bug Something isn't working label Apr 12, 2023
@ALongStringOfNumbers ALongStringOfNumbers added this to the 2.6.2 milestone Apr 13, 2023
@bruberu bruberu merged commit 4baec8b into master Apr 30, 2023
@bruberu bruberu deleted the bru-defaultEnchantments2 branch April 30, 2023 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants