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

Refactor replaceAllOccurences & add progress bar when applying actions #1144

Merged
merged 24 commits into from Feb 7, 2021

Conversation

friendlyhj
Copy link
Contributor

@friendlyhj friendlyhj commented Jan 22, 2021

I think this PR can slightly increase efficiency of this method.

In my test in an instance with GTCE, it reduces the time taken from 28 seconds to 11 seconds

@friendlyhj friendlyhj changed the title Refactor replaceAllOccurences Refactor replaceAllOccurences & add progress bar when applying actions Jan 23, 2021
@friendlyhj
Copy link
Contributor Author

@jaredlll08 what's your thought?

@jaredlll08
Copy link
Member

I like the progress bar stuff, I think that is really cool.

I'm not sure about the replacealloccurances though, the whole method has just been trouble and I'm scared that if we change it now, it may cause even more trouble.

How certain are you that it works and all is fine with it?

@friendlyhj
Copy link
Contributor Author

friendlyhj commented Jan 24, 2021

It works well. Almost all recipes are corrently modified.

But in some recipes, there is an issue. If a recipe can use ingredient A or B (like IngredientOr ?), now I want to replace A with C. In previous implementation, the ingredient will be replaced to C. But In my implementation, it won't be replaced. The previous one maybe over-replace. But the my one cannot replace under certain condition. I don't know how to deal with it well. So I plan to add a function to let users determine when the ingredient should be replaced. (add a new method, the previous method will use previous matching way)

Now A or B will be replaced to C or B. It works perfectly now.

@friendlyhj
Copy link
Contributor Author

@jaredlll08 ready for review

@jaredlll08
Copy link
Member

Going forward with PRs, maybe mark them as a draft, and when you're ready for review just mark it as ready, just makes it easier to manage things.

I also want @kindlich's thoughts on it, but besides that it mostly looks good

Copy link
Member

@jaredlll08 jaredlll08 left a comment

Choose a reason for hiding this comment

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

Looks good

@friendlyhj
Copy link
Contributor Author

@jaredlll08 @kindlich when the PR is merged? I'm developing a modpack that needs the function.

Copy link
Collaborator

@kindlich kindlich left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I added a few Codestyle suggestions to make SubActionReplaceAllOccurrences#apply easier to read ^^

Also, as a side note:
This implementation creates a different recipe name if a recipe is affected by multiple replaceAllOccurences calls. Prior implementation would append remodified, reremodified etc., This implementation will only append modifed.
That is fine by me, I just wanted to point it out.

Also, the previous implementation used IIngredient#contains(IIngredient) to match, whereas you do it manually and allow partial replacements. This can lead to different results if I'm not mistaken.
That is fine by me, I just wanted to point it out.

@friendlyhj
Copy link
Contributor Author

friendlyhj commented Feb 6, 2021

SubActions only modify the recipe pattern, the ingredient array. After all sub actions are applied, if the ingredient array is modified (ActionReplaceAllOccurence.INSTANCE.recipeModified == true), the recipe will be modified. so, the recipe is modified only once. There is no reason for a reremodified recipe.
About these code style suggestion, I will change them tomorrow later.

@jaredlll08 jaredlll08 merged commit 41c43b7 into CraftTweaker:1.12 Feb 7, 2021
@friendlyhj friendlyhj deleted the refactor branch February 11, 2021 07:54
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