AntMCDev / MCSkyblock

The Official MCSkyblock repository
MIT License
6 stars 2 forks source link

[Future] Move code to official mojang mappings. #23

Closed JosephMillsAtWork closed 2 years ago

JosephMillsAtWork commented 2 years ago

Advantages

Est Time to port 5.6 hours.

I will do this tomorrow and send a pull request.

JosephMillsAtWork commented 2 years ago

@AntMCDev this has been added feel free to test out and give feedback. It should be out of WIP by tomorrow.

AntMCDev commented 2 years ago

Ok, I've glanced over the source changes and given that a very brief test. I definitely think it's an improvement at least in terms of maintainability, however there's a few reasons the way things were done the way they were (not saying there's no better alternatives). Firstly, disabling structures altogether isn't what the current mod does - it disables the block placement but leaves the underlying structure in place (this allows for witch farms / wither skeleton farms etc), whereas in the fork structures are disabled. Possible this just a non-implemented feature at present, I can see StructureTracker::isEnabled does not appear to have usages at present (edit: actually it does, but the static method is referenced via an object instance - that should be referencing the class itself, not an instance).

Secondly, the structure checker network packet was removed. The reason that feature was in the original is similar to the reason above - structures technically still spawn, but without the comprising blocks so it's a useful way to tell if you're within the bounds of a structure.

Thirdly (and I know you've flagged this as FIXME in the source) - the end doesn't currently generate as a void (when setting CUSTOM_END to true - certain features generate). The way I solved this in the old source was via Mixins (to remove the features and fix the Y positioning of the end portal). Also the end is currently the legacy single biome source, even in MC's source if I recall correctly - but might be able to update that to a multi biome noise source if we wanted.

Other than the point above I definitely think it's a step in the right direction. The original source hasn't really been refactored at all and started as a Forge project many versions ago - and never really got proper update. Finally, with regards to the villager mod idea, I definitely think that's a good idea - pretty starved for time at the moment, but if that's a feature that you wanted to implement, I'd be more than happy for that to happen. I've added you as a collaborator on the main repo - if you wanted to make use of that. There's a develop-1.19 branch that you should be able to push directly to. Alternatively, if you wanted to keep it as a fork / separate CurseForge project, I'm happy to link to it from the main page of the current mod

AntMCDev commented 2 years ago

Bug with the fork - seems like the only biome that currently generates in the overworld is river. Likewise chunks aren't generating in the nether at all (debug GUI just says "Waiting for chunk"). Vaguely remember having this bug on the main repo before a long time ago, so possible whatever fixed that got removed

JosephMillsAtWork commented 2 years ago

Point taken and thanks for adding me to the project.

I have started on the wiki and added a new issue based on your feedback. Here is some notes about your feedback.

1) StructureTracker::isEnabled is in the generator on create structure method. But is still being worked on today, Your point about the bounding boxes is noted. Let me think about that today and I will get back to you.

2) I will add it back. but most the time I use the mod "minihud" for this. it is also useful for showing the biomes(rendered)

3) I didn't get to the end last yesterday. I was thinking about it and wanted to talk to you about the idea of maybe after X amount of blocks and what not. main island vs outer edge ect.

4) The bug with the biomes is noted and I just patched it. Im thinking about adding our own biome source.

JosephMillsAtWork commented 2 years ago

@AntMCDev are you on discord ? Maybe we can chat there or even make a server for this project ?

JosephMillsAtWork commented 2 years ago

Biomes are Fixed at commit

db2121d1f31cacd729e26bc57ecf67bbd1144914

AntMCDev commented 2 years ago

Yeah, on the first point that was just the usages in IntelliJ being silly (it didn't recognise the usage as it was referencing a static method via an object instance). I haven't used minihud myself, but looking over the mod description it would indicate that it's client-only (not that it's too big of an issue), but if it does roughly the same thing, I'm fine for the structure packet logic to be scrapped in favour of that.

JosephMillsAtWork commented 2 years ago

Nether is also fixed at last commit.

JosephMillsAtWork commented 2 years ago

30eb5ebd1b6574ad9d74d2e43160036d717083be

replaces TheEndBiomeSource (if you enable skyblock in the end) and brings in a new biome manager for the end biome source (SkyBlockEndBiomeSource). It disables the small end islands biome till I refactor it and remove the islands.

JosephMillsAtWork commented 2 years ago

I figure this to now be closed in development branch