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

Added fences #572

Merged
merged 8 commits into from
Nov 11, 2023
Merged

Added fences #572

merged 8 commits into from
Nov 11, 2023

Conversation

GameJarne
Copy link
Contributor

This adds wood, stone and obsidian fences.

Copy link
Member

@ChristofferHolmesland ChristofferHolmesland left a comment

Choose a reason for hiding this comment

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

Looks good, except it's missing translations? You need to add entries for the tiles.

@GameJarne
Copy link
Contributor Author

I'll do that right now. Although en-us has more entries than the other translation files?

GameJarne

This comment was marked as duplicate.

stone and obsidian textures were broken + updated wood fence textures
Copy link
Member

@BenCheung0422 BenCheung0422 left a comment

Choose a reason for hiding this comment

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

I think it should be allowed to place wooden fences on planks. Furthermore, I think all the fences should be allowed to be placed on grass tiles and floors with their corresponding materials.
Is it possible to add fence gates? I think it would be better if there is a door or gate that can allow players and entities passing through without breaking the tiles whilst having the similar textures with the fences.
The way to handle fence textures is clever, it is good.

@ChristofferHolmesland ChristofferHolmesland merged commit d764658 into MinicraftPlus:main Nov 11, 2023
2 checks passed
@Makkkkus Makkkkus mentioned this pull request Nov 15, 2023
@@ -76,6 +76,9 @@ public static void initTileList() {
tiles.put((short)47, new BossWallTile());
tiles.put((short)48, new BossFloorTile());
tiles.put((short)49, new BossDoorTile());
tiles.put((short)50, new FenceTile(Tile.Material.Wood));
tiles.put((short)51, new FenceTile(Tile.Material.Stone));
tiles.put((short)52, new FenceTile(Tile.Material.Obsidian));
Copy link
Member

Choose a reason for hiding this comment

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

Using there ids two times is absolutely catastrophic!

Copy link
Member

Choose a reason for hiding this comment

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

You are aware that this is because that it was made using another outdated version of the code therefore it’s technically correct at that time. However now it has to be adjusted

Copy link
Member

Choose a reason for hiding this comment

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

The point of a merge is to adjust the written code to reflect changes upstream. It wouldn't be hard to fix.

[
"has_plank"
]
],
Copy link
Member

Choose a reason for hiding this comment

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

Not quite sure why this is here. I assume this will break the code.

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't break the code as it follows the format

@@ -35,16 +35,19 @@ protected static ArrayList<Item> getAllInstances() {
items.add(new TileItem("Plank", new LinkedSprite(SpriteType.Item, "plank"), new TileModel("Wood Planks"), "hole", "water", "cloud"));
items.add(new TileItem("Plank Wall", new LinkedSprite(SpriteType.Item, "plank_wall"), new TileModel("Wood Wall"), "Wood Planks"));
items.add(new TileItem("Wood Door", new LinkedSprite(SpriteType.Item, "wood_door"), new TileModel("Wood Door"), "Wood Planks"));
items.add(new TileItem("Wood Fence", new LinkedSprite(SpriteType.Item, "wood_fence"), new TileModel("Wood Fence"), "grass"));
Copy link
Member

Choose a reason for hiding this comment

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

What about planks, sand, etc?

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.

5 participants