search

gigaherz / Ender-Rift

The Powered Bottomless Storage System
https://www.curseforge.com/minecraft/mc-mods/ender-rift
BSD 3-Clause "New" or "Revised" License
12 stars 6 forks source link

Improved the rift storage #56

Closed Lauriichan closed 1 year ago

Lauriichan commented 2 years ago

This PR introduces a more performance friendly storage system for the rift inventory It also allows to have more than 256 rifts (the old implemention used getByte("Rift") to get the id) All rifts have a UUID instead of an numeric id and their own file making it easier to get rid of them

This PR also adds a random item generator block for debugging purposes if needed I can strip it again To test stuff using this mod I added PipeZ, Spark, SolarGeneration and EdivadLib for testing, I commented for the time being

Lauriichan commented 2 years ago

It seems your changes also include changing the formatting style of the code, making reviewing the changes almost impossible due to the unnecessary style changes all over the place. And frankly, I have my style preferences which I would rather keep.

That's mostly my auto formatter yeah, I didn't edit all files so it mostly only includes all files I edited If you tell me more about the style you would like to see I can work on that

EDIT: If you use eclipse's auto formatter I can also just import your settings if you could export them for me

gigaherz commented 2 years ago

That's mostly my auto formatter yeah, I didn't edit all files so it mostly only includes all files I edited If you tell me more about the style you would like to see I can work on that

Main issue is the brace location, which I prefer in its own line. But I'd also ask that your PR should not modify any code that you didn't change, and that any new code should be consistent with the code around it. Even if that requires disabling parts of the auto-formatter.

EDIT: If you use eclipse's auto formatter I can also just import your settings if you could export them for me

I do not, I use Intellij IDEA, with mostly the default settings except for the things I changed once upon a time to match my preferences.

Lauriichan commented 2 years ago

But I'd also ask that your PR should not modify any code that you didn't change, and that any new code should be consistent with the code around it. Even if that requires disabling parts of the auto-formatter.

Yeah as I said no problem for me I'll change it rq

Lauriichan commented 2 years ago

I do not, I use Intellij IDEA, with mostly the default settings except for the things I changed once upon a time to match my preferences.

If the style isn't fine like this you could send me your IntelliJ IDEA settings and then I'll format it with that one once

gigaherz commented 2 years ago

You'll notice that you still have A WHOLE LOT of differences. I didn't notice it before (because they are invisible), but your settings also replaced all my indentations by tabs. I prefer spaces.

Default_1.xml.txt

But let me be more direct with this: I do not want you to find an autoformatter preset that is "close" to what I have, because my IDE settings don't do things like split long lines automatically, and I don't run the auto-formatting, so many of my lines of code are just "as I wrote them". The only settings the IDE changes automatically are brace locations, indentation, and spaces in/around operators and keywords.

Your new code should use my choices with regards to brace locations and indentation type and leave every other line in the file as it was before. If you didn't modify a line of code, it should not exist in the code diff.

EDIT: clarified a few things.

Lauriichan commented 2 years ago

If you didn't modify a line of code, it should not exist in the code diff.

Well easier said than done if I'm being honest after making my changes but I'll try my best to fix it

gigaherz commented 2 years ago

Well easier said than done if I'm being honest after making my changes but I'll try my best to fix it

What I would suggest you do:

  1. reformat your code to 4 spaces and braces in their own lines.
  2. In your favorite git client, use the option to "show diff between" / "show changes from" or whatever you client has, and choose the commit you used as a basis for the PR (the latest commit in the main repository, presumably). And go change by change, and apply the old version of the lines you didn't modify yourself.
gigaherz commented 2 years ago

(sorry if anything I say comes through as rude or specific, it's not my intention, but I am halfway to a headache/migraine)

Lauriichan commented 2 years ago

(sorry if anything I say comes through as rude or specific, it's not my intention, but I am halfway to a headache/migraine)

Don't worry x) I understand that someone just comming and changing your code style can be very annoying and it was not my intention to hurt anyone when adding this feature ^^ As I said I'll fix it x)

Lauriichan commented 2 years ago

Okay now it should be fine ^^

EDIT: I would recommend fixing the model of the debug_item_block block model cause I've no clue how to work with textures and models with forge Also I didn't increase the version so that would be something that you would need to take care of

gigaherz commented 2 years ago

I have started reviewing the code, but I am also in the middle of making supper so I will continue later.

Lauriichan commented 2 years ago

Okay, take your time ^^ Theres no need to hurry or anything especially because its your mod

Lauriichan commented 2 years ago

Hey there just wanted to check how things are going since a week already passed

gigaherz commented 2 years ago

Oof sorry my ability to focus has gone to shit. I'll try to remember over the weekend.

Lauriichan commented 2 years ago

Hey there I'm guessing you forgot about it again or had no time so I wanted to remind you of this PR again :)

gigaherz commented 2 years ago

I uh.... haven't forgotten, just haven't found a combination of remembering, having time, and having energy. Sorry >_<

Lauriichan commented 2 years ago

Well yeah that's understandable as well no worries ^^

Lauriichan commented 1 year ago

Hey there its about 3 months now wanted to remind you of the PR because of that if you might have already forgotten about it ^^ If you still didn't have the time that's fine but I still wanted to remind you

gigaherz commented 1 year ago

Oh, heck. I did forget this time. My situation hasn't really changed since September -- I have basically not been doing anything coding-related at all outside my day job. I had some concerns with some of the changes, which I still haven't figured out. But I'll merge now, and then figure things out locally. Expect some of your changes to get tweaked before I release this to the public. :P

Lauriichan commented 1 year ago

Oh, heck. I did forget this time. My situation hasn't really changed since September -- I have basically not been doing anything coding-related at all outside my day job. I had some concerns with some of the changes, which I still haven't figured out. But I'll merge now, and then figure things out locally. Expect some of your changes to get tweaked before I release this to the public. :P

Feel free to change whatever you like :) I just wanted to help fix the lag issues and implemented it in a way that it should be better than before. There were some things that I didn't understand of your systems so I tried to minimize the amount of changes to existing code as I didn't want to mess things up.

Also if you have any questions related to the code feel free to contact me per mail or if you want we could also message on discord