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.

Sign up for GitHub

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

Jump to bottom

Methods in TheEndBiomeData that query the intended type. #2369

Open
wants to merge 13 commits into
base: 1.19
Choose a base branch
from
Open

Methods in TheEndBiomeData that query the intended type. #2369

wants to merge 13 commits into from

Conversation

quiqueck
Copy link

When implementing a custom BiomeSource for the End, we need to know what the intended placement of an End-biome was. This PR adds Methods to TheEndBiomeData that are similar to NetherBiomes.canGenerateInNether.

@quiqueck quiqueck mentioned this pull request Jun 30, 2022
Closed
quiqueck added a commit to quiqueck/BCLib that referenced this pull request Jun 30, 2022
@quiqueck
Better Abstraction from internal FAPI (#2, FabricMC/fabric#2369)
59756e6
@apple502j
Copy link
Contributor

I am mot sure if the "intended" naming is good.

@quiqueck
Copy link
Author

We could use canGenerateAs instead (similar to NetherBiomes.canGenerateInNether).

deirn
deirn reviewed Jul 2, 2022
View reviewed changes
fabric-biome-api-v1/src/main/java/net/fabricmc/fabric/impl/biome/TheEndBiomeData.java Outdated
Comment on lines 105 to 114
public static boolean isIntendedForEndMidlands(RegistryKey<Biome> biome){
return END_MIDLANDS_MAP.containsKey(biome);
}

/**
* Returns true if the given biome was added as barrens biome in the end, considering the Vanilla end biomes,
* and any biomes added to the End as barrens biome by mods.
*/
public static boolean isIntendedForEndBarrens(RegistryKey<Biome> biome){
return END_BARRENS_MAP.containsKey(biome);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem to be the correct way to do it, the midlands and barrens map has the highland biome as the key.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, just realized that here quiqueck/BCLib#13

@deirn deirn added enhancement New feature or request small change priority:medium Medium priority PRs that should get reviews labels Jul 2, 2022
@deirn deirn requested a review from a team July 2, 2022 05:00
@apple502j
Copy link
Contributor

I think it would be better if WeightedPicker stored the entries as sets, etc. @deirn what do you think?

@quiqueck quiqueck changed the title Methods inTheEndBiomeData that query the intended type. Methods in TheEndBiomeData that query the intended type. Jul 2, 2022
@quiqueck quiqueck marked this pull request as draft July 2, 2022 08:32
@quiqueck quiqueck marked this pull request as ready for review July 2, 2022 08:42
@deirn
Copy link
Member

deirn commented Jul 2, 2022

We could use canGenerateAs instead (similar to NetherBiomes.canGenerateInNether).

It's still too verbose imo, I'd just named it like the add methods, e.g. isMidlandsBiome or so.

I think it would be better if WeightedPicker stored the entries as sets, etc. @deirn what do you think?

I'm not really sure how WeightedPicker works tbh, but it looks like it needs to be a list?

deirn
deirn reviewed Jul 2, 2022
View reviewed changes
fabric-biome-api-v1/src/main/java/net/fabricmc/fabric/impl/biome/WeightedPicker.java Outdated
Comment on lines 110 to 112
Optional<WeightedEntry<T>> getAny(T element){
return entries.stream().filter(w -> w.entry==element).findAny();
}
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this, you might want to look into changing TheEndBiomeData#ADDED_BIOMES into a Map<RegistryKey<Biome>, RegistryKey<Biome>> instead, with vanilla biome as its value. Then you can check if the value is equal to the vanilla biome.

Patch file

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that is much cleaner. I'll include the suggested patch...

@quiqueck
Copy link
Author

quiqueck commented Jul 2, 2022

I think it would be better if WeightedPicker stored the entries as sets, etc. @deirn what do you think?

I'm not really sure how WeightedPicker works tbh, but it looks like it needs to be a list?

I think the search-method needs a list for deterministic results.

deirn
deirn approved these changes Jul 2, 2022
View reviewed changes
Copy link
Member

@deirn deirn left a comment

Choose a reason for hiding this comment

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

Few checkstyle issues, other than that, LGTM.

@deirn deirn requested a review from a team July 2, 2022 15:10
@apple502j
Copy link
Contributor

@quiqueck The suggestion was to use both a set and a list. Anyway, seems like it was solved.

Technici4n
Technici4n reviewed Jul 5, 2022
View reviewed changes
Copy link
Member

@Technici4n Technici4n left a comment

Choose a reason for hiding this comment

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

Could there be a case where a biome replaces multiple end biomes, and does that need to be accounted for?

Looks good otherwise.

apple502j
apple502j suggested changes Jul 30, 2022
View reviewed changes
Copy link
Contributor

@apple502j apple502j left a comment

Choose a reason for hiding this comment

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

See the review above - as far as I understand one biome can in fact replace multiple biomes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Technici4n Technici4n Technici4n left review comments

@apple502j apple502j apple502j requested changes

@deirn deirn deirn approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request priority:medium Medium priority PRs that should get reviews small change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@quiqueck @apple502j @deirn @Technici4n