-
Notifications
You must be signed in to change notification settings - Fork 89
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
asteroid dimension #320
base: main
Are you sure you want to change the base?
asteroid dimension #320
Conversation
Added Asteroids Dimension and configured skybox. Haven't started terrain generation yet.
Completed Asteroid Dimension Terrain Generation Structures/Features not implemented yet
Canned food items can now be created from any food with ease. planning to add automatic food detection that creates cans for any mods foods.
canned food items have been almost completed other than fixing appleskins api integration and creating a machine to make cans in survival
canned food items completed - added total nutrition - fixed whitespace - added appleskin integration for canned food Todo - create canning machine to make canned food accessible in survival
food canner fully implemented and works completely. also put licencensing on all new files i have created
sort of started working on asteroid dimension structures
removed TODO: comment in food_canner json because it was causing issues. make sure in future to change the recipe for the food canner block ad the advancement requirements added licencing
fixed json issue added only eat canned food in space sealer code half implemented. uploading github because need backup because rewriting sealer AGAIN...
- added asteroid ores + dense ice - fixed some data generation issues - made asteroid dimension pitch black - fixed air lock seal for the second time - fixed canned food textures
- Fixed translations for canned food - Fixed ores aswell because asteroid ores where broken
Launch pads are now dynamic and are created using one method in the RocketParts class
added Rocket Nose Cone items
This reverts commit 697ed02.
This reverts commit 8087c7e.
This reverts commit 838e1cb.
decided to leave rocket code as is because rebuilding everything from scratch was painful. just going to build ontop of the already implemented system
Updated everything to work with 1.21
4c5d2c2
to
8e8a2e2
Compare
Rockets? |
fixed most generated files and moved some to resources
Fixed most of the bugs with the oxygen sealer. Few more to fix but need to fetch the main branch first to fix conflicts
Sealers are now at a stage where they are usable in the game. They arent perfect but they are usable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked over most things, but there is a lot, so my comments are by no means exhaustive. Hopefully this should be enough for you to get started on making some changes. I do think there is a fair amount of redundancy which you could work on reducing, so maybe the next time someone looks there won't be nearly so much to review.
...main/generated/data/galacticraft/advancement/recipes/building_blocks/raw_aluminum_block.json
Outdated
Show resolved
Hide resolved
src/main/java/dev/galacticraft/mod/client/gui/screen/ingame/FoodCannerScreen.java
Outdated
Show resolved
Hide resolved
src/main/java/dev/galacticraft/mod/client/gui/screen/ingame/OxygenSealerScreen.java
Show resolved
Hide resolved
src/main/java/dev/galacticraft/mod/content/item/CannedFoodItem.java
Outdated
Show resolved
Hide resolved
src/main/java/dev/galacticraft/mod/data/GCTranslationProvider.java
Outdated
Show resolved
Hide resolved
src/main/java/dev/galacticraft/mod/data/GCTranslationProvider.java
Outdated
Show resolved
Hide resolved
src/main/resources/assets/galacticraft/textures/gui/food_canner_screen.png
Outdated
Show resolved
Hide resolved
code merge updates
dont know what the problem was but i just removed the file and ran datagen again and it was fixed
didnt realise how %s works
total nutrition wasnt displayed correctly due to wrong prefix of item instead of ui
airlock became broken at some point so i fixed it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an extremely large patch so I haven't gone over everything quite yet, but it's probably helpful to get this review in sooner than later (feel free to message me if you have any questions!)
Some general things:
- Might need to check files for the correct line endings
- Optimize imports should be run (via IntelliJ/your IDE)
- License headers need updating (
gradlew updateLicenses
) - The curly brace placement should be consistent (placement on the same line)
I'm not super sold on the GC4 asteroid generation code - I can't say I understand how that code actually works, and the external save data system is slightly cursed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this file was changed to use Windows line endings (CRLF).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line endings again
boolean mask = false; | ||
for (int i = 0; i < this.galacticraft$getAccessories().getContainerSize(); i++) { | ||
Item item = this.galacticraft$getAccessories().getItem(i).getItem(); | ||
if (!mask && item instanceof OxygenMask) { | ||
mask = true; | ||
} | ||
} | ||
return mask; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of mask = true
, I think its equivalent to just return true immediately after finding a mask, and false otherwise.
import org.jetbrains.annotations.ApiStatus; | ||
|
||
import javax.swing.event.ChangeEvent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be a good idea to run optimize imports across this patch. (I was very concerned when I saw this import)
@@ -39,6 +41,7 @@ | |||
|
|||
public interface Constant { | |||
String MOD_ID = "galacticraft"; | |||
public static final String GCDATAFOLDER = "../galacticraft/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final
is extraneous in an interface
import org.jetbrains.annotations.Contract; | ||
|
||
import java.util.List; | ||
import java.util.function.Supplier; | ||
|
||
import static dev.galacticraft.machinelib.api.block.MachineBlock.ACTIVE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import?
|
||
@Mixin(ClientTooltipComponent.class) | ||
public interface ClientTooltipMixin { | ||
@Overwrite |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Overwrite
s should be avoided as much as possible - I think fabric api's TooltipComponentCallback
should do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think DataComponents.FOOD
should be set on the canned food item, rather than overriding the component.
public void loadAdditional(CompoundTag tag, HolderLookup.Provider lookup) { | ||
super.loadAdditional(tag, lookup); | ||
setProgress(tag.getInt(Constant.Nbt.PROGRESS)); | ||
this.transferring_can = tag.getBoolean("TRANSFERRING_CAN"); | ||
this.transferring_food = tag.getBoolean("TRANSFERRING_FOOD"); | ||
ItemStack itemStack = GCItems.CANNED_FOOD.getDefaultInstance(); | ||
itemStack.set(DataComponents.BLOCK_ENTITY_DATA, CustomData.of(listTagToCompoundTag(tag.getList("STORAGE", ListTag.TAG_COMPOUND)))); | ||
this.storage = itemStack; | ||
|
||
} | ||
|
||
|
||
@Override | ||
public void saveAdditional(CompoundTag tag, HolderLookup.Provider lookup) { | ||
super.saveAdditional(tag, lookup); | ||
tag.putInt(Constant.Nbt.PROGRESS, this.progress); | ||
tag.putBoolean("TRANSFERRING_CAN", this.transferring_can); | ||
tag.putBoolean("TRANSFERRING_FOOD", this.transferring_food); | ||
tag.put("STORAGE", compoundTagToListTag(Objects.requireNonNull(this.storage.get(DataComponents.BLOCK_ENTITY_DATA)).copyTag())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NBT tag names should be PascalCase
, fields should be in camelCase
import java.util.*; | ||
|
||
public class SealerManager { | ||
public static final SealerManager INSTANCE = new SealerManager(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to attach this to a Level
instead of a global state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had already started this review before Marcus made his, but I have identified some different things and a possible solution to your Windows line ending problem with the .gitattributes
and .gitignore
files.
I can work with you on updating the worldgen rather than just porting GC4 code too.
.gitattributes
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run the comand git checkout <sha> .gitattributes
where <sha>
is the hash of the latest commit to main to prevent the .gitattributes
file from being updated without any actual changes to avoid confusing the Git history.
.gitignore
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any actual changes to this file? If not, could you run the comand git checkout <sha> .gitignore
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than deleting this can you just revert it back to what it was before so that the raw aluminum block can still be crafted.
You could use the git checkout <sha> src/main/.../raw_aluminum_block.json
command for this as well.
@@ -27,6 +27,8 @@ | |||
import net.minecraft.ChatFormatting; | |||
import net.minecraft.core.Direction; | |||
import net.minecraft.core.Registry; | |||
import net.minecraft.data.models.model.ModelTemplate; | |||
import net.minecraft.data.models.model.TextureSlot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unused imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The brackets in this file need updating. The license header is also out-of-date (2019-2025).
"bottom": "galacticraft:block/tin_decoration" | ||
}, | ||
"elements": [ | ||
{ "from": [ 4, 0, 0 ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New line, please.
if (isCannedFoodItem(stack)) { | ||
//data component holder has parent of canned food item | ||
if (type.equals(DataComponents.FOOD)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this bracket on the line before.
{ | ||
return InteractionResultHolder.pass(heldItem); | ||
} else { | ||
player.displayClientMessage(Component.translatable("chat.cannotEatWithMask").withColor(Color.RED.getRGB()), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this translation key in the Translations class and use .withStyle(Constant.Text.RED_STYLE)
or .withStyle(ChatFormatting.RED)
instead of .withColor(Color.RED.getRGB())
.
} | ||
} | ||
} else if (!oxygenWorld && !world.isBreathable(new BlockPos((int) Math.floor(playerEyePos.x), (int) Math.floor(playerEyePos.y), (int) Math.floor(playerEyePos.z)))) { //sealed atmosphere check. they dont have a mask on so make sure they can breathe before eating | ||
player.displayClientMessage(Component.translatable("chat.cannotEatInNoAtmosphere").withColor(Color.RED.getRGB()), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this translation key in the Translations class too.
import org.jetbrains.annotations.Nullable; | ||
|
||
import java.awt.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere?
Fixed some textures plus added asteroid dimension