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

Evenly divide generated energy over all connected energy networks #3943

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

iTwins
Copy link
Contributor

@iTwins iTwins commented Aug 10, 2023

Description

When a generator was connected to multiple networks it supplied its full generated output to all connected networks.

Proposed changes

Evenly divide generated energy over all connected energy networks. output / networks.size * networks.size = output, so the generator will now produce the correct amount of energy when connected to multiple networks.

Related Issues (if applicable)

Resolves #3260

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@iTwins iTwins requested a review from a team as a code owner August 10, 2023 12:40
@github-actions
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility

If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@github-actions
Copy link
Contributor

github-actions bot commented Aug 10, 2023

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 6e91c98f

https://preview-builds.walshy.dev/download/Slimefun/3943/6e91c98f

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

@Sefiraat
Copy link
Member

What happens when an energy Network is fully saturated with power, I wouldn't expect the output of power to be divided like this if only one of the connected Networks is able to accept power?

@JustAHuman-xD
Copy link
Contributor

What happens when an energy Network is fully saturated with power, I wouldn't expect the output of power to be divided like this if only one of the connected Networks is able to accept power?

Oooh that's a great point, glad you brought that up

@iTwins
Copy link
Contributor Author

iTwins commented Aug 10, 2023

Hmm. Yes. It'll still give the saturated energy network it's 'share' but that energy will just be voided. That definitely needs to be fixed.

@iTwins
Copy link
Contributor Author

iTwins commented Aug 10, 2023

Fixed, right now it divides only among non saturated networks. However the supply holograms for the saturated networks show the supply the non saturated networks are getting. I was wondering if they should be like this or if they should display the supply they would be getting if they weren't saturated.

@JustAHuman-xD JustAHuman-xD added 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. ✨ Fix This Pull Request fixes an issue. labels Aug 11, 2023
JustAHuman-xD
JustAHuman-xD previously approved these changes Aug 11, 2023
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM but want to get some more testing on this before we send it 👍

@Phoenix-Starlight
Copy link
Member

What if the connected energy network count exceeds the generator's power rate? It'll become 0. Not sure whether that is desired or not, but it could be an intended mechanic perhaps.

@iTwins
Copy link
Contributor Author

iTwins commented Aug 11, 2023

I would say that would be intended. If there are more networks connected to a generator that the amount of the Joules it generates, each network would get <1 Joule which is floored to 0. You could also ceil the result of the division. The difference would be 1 Joule per network (max 6) which isn't very significant.

@JustAHuman-xD
Copy link
Contributor

I don't think the drop off is a problem. If we were to min it at 1 then that could lead to low level duplication if a generator only generated like 1-5 joules and gets split 6 ways.

Plus if we want to give reasoning we can just say it's energy resistance being more an issue when using multiple regulators

Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM

@JustAHuman-xD
Copy link
Contributor

Anyone up for testing this 👀

@Boomer-1
Copy link

this still needs work. in placing 4 regulators and a lava generator, while all 4 went up nearly even, it was well above the rate that the lava generator produced. each went up 8 j/t, while the gen makes 10 total per tick, which created an excess of 22 per tick. additionally it sped up how fast the generator used up the lava. it's supposed to last 40 seconds, it lasted 10. then the amount of power in each network was way above what it should have been. this needs a lot more work

@JustAHuman-xD
Copy link
Contributor

JustAHuman-xD commented Nov 23, 2023

this still needs work. in placing 4 regulators and a lava generator, while all 4 went up nearly even, it was well above the rate that the lava generator produced. each went up 8 j/t, while the gen makes 10 total per tick, which created an excess of 22 per tick. additionally it sped up how fast the generator used up the lava. it's supposed to last 40 seconds, it lasted 10. then the amount of power in each network was way above what it should have been. this needs a lot more work

Sooooooo not sure how you got those results. This PR can not affect how long the lava lasts. And I've tested this previously without issue. Are you sure you used the right tier of lava generator?

@iTwins
Copy link
Contributor Author

iTwins commented Nov 23, 2023

I found out why. The pr didn't change how long the lava lasts, this was already the behavior before this pr. As for the excess power, that is because I only tested this for machines with no buffer 😅. The generated power is divided evenly floor(10/4) then stored in the buffer 4 times meaning the buffer increases by 8 every tick. But the whole buffer is still accessible by all networks.
The easiest fix imo is keeping EnergyNetProviders that implement MachineProcessHolder<FuelOperation> the same as they are now (4x the full output but fuel goes 4x faster) and EnergyNetProviders that do not will have their generated output evenly distributed. And if generators have a buffer also evenly distribute that.
The best fix imo would be changing the fuel consumption rate back to expected amount and evenly distributing it aswell, however this would be harder to make.

@JustAHuman-xD
Copy link
Contributor

this still needs work. in placing 4 regulators and a lava generator, while all 4 went up nearly even, it was well above the rate that the lava generator produced. each went up 8 j/t, while the gen makes 10 total per tick, which created an excess of 22 per tick. additionally it sped up how fast the generator used up the lava. it's supposed to last 40 seconds, it lasted 10. then the amount of power in each network was way above what it should have been. this needs a lot more work

The reason the lava went 4x as quick was because each energy net ticked it down. That happens regardless of this pr like iTwins said. Honestly I think that should be the behavior

@Boomer-1
Copy link

this still needs work. in placing 4 regulators and a lava generator, while all 4 went up nearly even, it was well above the rate that the lava generator produced. each went up 8 j/t, while the gen makes 10 total per tick, which created an excess of 22 per tick. additionally it sped up how fast the generator used up the lava. it's supposed to last 40 seconds, it lasted 10. then the amount of power in each network was way above what it should have been. this needs a lot more work

The reason the lava went 4x as quick was because each energy net ticked it down. That happens regardless of this pr like iTwins said. Honestly I think that should be the behavior

that would be fine if the power spread evenly as it should, and the total produced matched what the fuel would produce if just on one network

@JustAHuman-xD
Copy link
Contributor

iTwins if you can vc in a little bit to talk about some of this / make the changes necessary I'd be down.

@iTwins iTwins marked this pull request as draft December 6, 2023 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Fix This Pull Request fixes an issue. 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generators power multiple energy network with unnerfed rate
6 participants