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

Apply correct EU modifier for calculating parallels in the Volcanus. #3754

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Vlamonster
Copy link
Contributor

@Vlamonster Vlamonster commented Jan 6, 2025

Resolved issue #17399.

@Dream-Master
Copy link
Member

Dream-Master commented Jan 6, 2025

@Vlamonster
Star imports need to be reversed or it won’t build

@Dream-Master Dream-Master requested a review from a team January 6, 2025 23:03
@Dream-Master Dream-Master added the bug fix Fix a bug. Please link it in the PR. label Jan 6, 2025
@Dream-Master Dream-Master added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label Jan 8, 2025
Comment on lines +269 to +271
.setEUtModifier(
euModifier
* Math.pow(0.95F, Math.floor((getCoilLevel().getHeat() - recipe.mSpecialValue) / 900F)))
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is now applying the heat EU/t discount twice (before and after parallel). Currently it seems like every machine that does a heat EU/t discount does this discount after parallel. This isn't necessarily a "bug," but a decision we need to make whether this discount should be applied before or after. If we decide to change it to before though, it should be changed deeper down in processinglogic rather than in a machine implementation like this

Copy link
Contributor Author

@Vlamonster Vlamonster Jan 8, 2025

Choose a reason for hiding this comment

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

This behavior exists in all other machines afaik, and we only noticed a difference in the Volcanus because we use a spreadsheet to calculate overclocks for all machines up to UMV.

Copy link
Member

Choose a reason for hiding this comment

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

This is not the behavior of other machines with heat discounts, MEBF, EBF, Vacuum Furnace, Gorge EBF modules all use similar heat discount mechanics but do not double apply like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by double apply? It doesn't provide a double discount if that's what you're referring to. It just makes it so that it uses all available energy for parallels, i.e. if it can do another parallel then it will.

Copy link
Member

@serenibyss serenibyss Jan 15, 2025

Choose a reason for hiding this comment

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

The heat discount is applied during overclock calculation, here

double heatDiscountMultiplier = calculateHeatDiscountMultiplier();

As said in the Java docs of OverclockCalculator#setHeatDiscount, "Sets if we should add a heat discount at the end of calculating an overclock, just like the EBF." Volcanus does this discount after parallels during overclock, like EBF, MEBF, etc. so by adding this discount here you are doing this heat discount twice, once before parallel and once after.

Furthermore, if this change is to be accepted, the heat discount should be applied before parallel internally, instead of needing to do a hack like this PR. Though this needs discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Fix a bug. Please link it in the PR. 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants