Skip to content

Conversation

@IncredibleHolg
Copy link
Contributor

All blocks needed for the warped and cromson biomes are now implemented.

@IncredibleHolg
Copy link
Contributor Author

@CounterPillow I would like to go on with logs, slabs and stairs, as soon as thus is merged.

Copy link
Member

@CounterPillow CounterPillow left a comment

Choose a reason for hiding this comment

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

Looks mostly okay aside from the large if-ery in the wood blocks, which I'd appreciate if you replaced into a dictionary lookup for easier future maintenance and less branching

Comment on lines +1176 to +1201
if blockid == 1008: # nether logs aka stem
if wood_type == 0: # warped_stem
top = self.load_image_texture("assets/minecraft/textures/block/warped_stem_top.png")
side = self.load_image_texture("assets/minecraft/textures/block/warped_stem.png")
if wood_type == 1: # stripped_warped_stem
top = self.load_image_texture("assets/minecraft/textures/block/warped_stem_top.png")
side = self.load_image_texture("assets/minecraft/textures/block/stripped_warped_stem.png")
if wood_type == 2: # crimson_stem
top = self.load_image_texture("assets/minecraft/textures/block/crimson_stem_top.png")
side = self.load_image_texture("assets/minecraft/textures/block/crimson_stem.png")
if wood_type == 3: # crimson_stem
top = self.load_image_texture("assets/minecraft/textures/block/crimson_stem_top.png")
side = self.load_image_texture("assets/minecraft/textures/block/stripped_crimson_stem.png")
if blockid == 1009: # nether hyphae
if wood_type == 0: # warped_hyphae
top = self.load_image_texture("assets/minecraft/textures/block/warped_stem.png")
side = top
if wood_type == 1: # stripped_warped_hyphae
top = self.load_image_texture("assets/minecraft/textures/block/stripped_warped_stem.png")
side = top
if wood_type == 2: # crimson_hyphae
top = self.load_image_texture("assets/minecraft/textures/block/crimson_stem.png")
side = top
if wood_type == 3: # stripped_crimson_hyphae
top = self.load_image_texture("assets/minecraft/textures/block/stripped_crimson_stem.png")
side = top
Copy link
Member

Choose a reason for hiding this comment

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

these ifs only ever end up taking one branch, so checking all the others with further "if" conditions as opposed to elif is checking way more than it should for no reason. Ideally this would all just be refactored into a dictionary lookup though.

# 1 UNWSE
# 2 UWSEN
# 3 USENW

Copy link
Member

Choose a reason for hiding this comment

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

extraneous whitespace added

Comment on lines +4411 to +4415
soul_soil_texture="assets/minecraft/textures/block/soul_soil.png"
block(blockid=1020, top_image=soul_soil_texture, side_image=soul_soil_texture)
# nether gold ore
nether_gold_texture="assets/minecraft/textures/block/nether_gold_ore.png"
block(blockid=1021, top_image=nether_gold_texture, side_image=nether_gold_texture)
Copy link
Member

Choose a reason for hiding this comment

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

There's no need to specify the same image as both top_image and side_image, if side_image is missing, then it takes on the value of top_image.

@CounterPillow
Copy link
Member

If you'd prefer I can merge this right now and fix the various nitpicks I had with a followup commit

@CounterPillow CounterPillow merged commit ba17369 into overviewer:116-blocks Aug 5, 2020
@IncredibleHolg IncredibleHolg deleted the my-116-blocks branch August 6, 2020 20:14
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.

2 participants