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

Adds a blacklist to prevent certain dims like Twilight Forest from generating overworld ores #3438

Conversation

spacebuilder2020
Copy link
Contributor

@spacebuilder2020 spacebuilder2020 commented Oct 31, 2024

This is to prevent mixes that are in both dims from not generating in the tf.
@Dream-Master Dream-Master requested a review from a team November 1, 2024 00:13
@chochem chochem added the bug fix Fix a bug. Please link it in the PR. label Nov 1, 2024
@Edgars-Cirulis
Copy link

anything new on it?

@Dream-Master
Copy link
Member

@chochem does it work ?

@chochem
Copy link
Member

chochem commented Nov 3, 2024

@chochem does it work ?

I had no time to test this.
(and I think its still unfinished? their reply to my comment was strange.

@spacebuilder2020
Copy link
Contributor Author

I also have not had time to test it. I will try to give it a test tonight.

@spacebuilder2020
Copy link
Contributor Author

Putting this into draft until I can figure out how best to properly integrate the config changes in https://github.com/GTNewHorizons/GT5-Unofficial/pull/2939/files#diff-2e797dc752a45ed5fc0dcde68cf95a8282c0d904decc64a1e9dfedaf830d12b6R105 and #3003 without hard coding a dim or a provider in the base class. Any provider logic should be handled in the sub classes and passed in the provider class list.

@spacebuilder2020 spacebuilder2020 marked this pull request as draft November 4, 2024 01:09
@chochem
Copy link
Member

chochem commented Nov 12, 2024

is there any update? we kind of need this. If you just want to look at the other parts later that is totally fine.

@spacebuilder2020
Copy link
Contributor Author

I have been trying to take a break from GTNH but in the interest in getting this done, I can take a look later today to see what I can do to wrap it up. I think theoretically, the hardcoded dim stuff should not break anything ( if (ore.twilightForest && aWorld.provider.dimensionId == 7) {) that wasn't already broken but they don't really belong in the isDimAllowed method since they are more configuration changes.

@wlhlm
Copy link
Member

wlhlm commented Nov 12, 2024

I have been trying to take a break from GTNH but in the interest in getting this done

Please don't feel pressured working on GTNH. If you want to take a break then that's fine. Just let us know, in case somebody else should take a stab (this is one of the bigger issues we need to fix for 2.7).

@spacebuilder2020
Copy link
Contributor Author

I have been trying to take a break from GTNH but in the interest in getting this done

Please don't feel pressured working on GTNH. If you want to take a break then that's fine. Just let us know, in case somebody else should take a stab (this is one of the bigger issues we need to fix for 2.7).

To be fair, the main reason I made this PR was I kinda got called out on the carpet in discord and didn't want my previous changes butchered or ripped out since they are rather essential for the operation of the Forge Essentials mod as well as any other mod with a multiworld module / plugin.

My current changes should work but changes in #3437 would have a similar effect as it stands now due to other changes as stated above effecting how this PR handles multiworlds and in both cases, multiworld is unusable in a twilight dim due to

if (ore.twilightForest && aWorld.provider.dimensionId == 7) {
. However, I have noticed in testing that for some reason a tf multiworld dim is always night instead of day in FE's multiworld anyway so it is probably a moot point.

I am going to take a stab at trying to fix it properly today since I think I have taken a long enough break anyway and I don't really like leaving this out here.

@chochem
Copy link
Member

chochem commented Nov 12, 2024

and yea absolutely: if you dont have plans to continue work on this that is totally fine. just let us know, then we know that we got to figure this out in the best possible way.

@spacebuilder2020
Copy link
Contributor Author

and yea absolutely: if you dont have plans to continue work on this that is totally fine. just let us know, then we know that we got to figure this out in the best possible way.

Sounds good. I will take a stab at it tonight and let you know tomorrow on what direction I plan to take.

@spacebuilder2020
Copy link
Contributor Author

Just had a chance to test updated logic. I am unable to find non-tf ores in the twilight forest. I am going to make another update to remove the hardcoded dim stuff and test that.

…ayer classes instead of in the base class as a hardcoded dim number.
@spacebuilder2020
Copy link
Contributor Author

Updated logic to remove hardcoded exceptions and tested to see if tf ores still generate in the tf.
2024-11-12_21 55 21

@spacebuilder2020 spacebuilder2020 marked this pull request as ready for review November 13, 2024 05:06
Copy link
Member

@chochem chochem left a comment

Choose a reason for hiding this comment

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

thank you!

@Dream-Master Dream-Master merged commit 82ae85b into GTNewHorizons:master Nov 14, 2024
5 checks passed
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mica Vein in Twlight Forest Twilight Forest contains Manganese vein
6 participants