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

moar biome settings 2: electric boogaloo #330

Merged
merged 12 commits into from
Jul 4, 2022

Conversation

duplexsystem
Copy link
Member

attempt 2 at pr

@dfsek
Copy link
Member

dfsek commented Jul 3, 2022

Next time can you not reformat all files, it makes reviewing very tedious.

@@ -1,5 +1,5 @@
# Magic options for more perf
org.gradle.jvmargs=-Xmx2048M -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 -XX:+UseParallelGC
org.gradle.jvmargs=-Xmx3048M -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 -XX:+UseParallelGC
Copy link
Member

Choose a reason for hiding this comment

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

3072

But is this increase even needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting OOM exceptions while building -- it's up to you on whether to include this but I will need to go back and change it locally every time

throw new IllegalStateException("Could not load all Terra classes", e);
}
} else {
// Forgive me for what I'm about to do...
Copy link
Member

Choose a reason for hiding this comment

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

This should not be in production code at all. Either put it in a completely separate source set, or remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see why it's an issue. As we've agreed I have the burden of maintaining the forge version I can tell you that the ability to run in a dev env is a pretty necessary thing for me. The code is only run if in a dev env and even if it somehow gets run instead of the normal awful hack it will just class not found. If it's moved into a different source set it will also class not found, there is practically no change to the end user. Moving it to another source set will just clutter up the source code. And it's not like the forge impl is the peak of correctness this is literally in AwfulForgeHacks. But if your deadset on it being in a different source set I will move it

Copy link
Member

Choose a reason for hiding this comment

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

I still dont like that its even there at all, but i guess there is no other option.

@duplexsystem
Copy link
Member Author

Next time can you not reformat all files, it makes reviewing very tedious.

my bad sorry, this was just optimize imports and I didn't remove the large diff in unrelated files, I did avoid reformatting everything though. However once this is merged it would be nice if you could reformat everything as especially my new code is a little messy

@dfsek dfsek merged commit f5da9d1 into PolyhedralDev:ver/6.2.0 Jul 4, 2022
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

2 participants