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

Implemented a Find Block operation #349

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Implemented a Find Block operation #349

wants to merge 7 commits into from

Conversation

Podshot
Copy link
Member

@Podshot Podshot commented Jun 27, 2021

This adds a new operation that finds a specific block (or all substates of a given block's namespaced name) and then reports the results in a tree control dialog with all of the coordinates

Example of the find block results dialog:
find_block_results

@Podshot Podshot added the type: enhancement New feature or request label Jun 27, 2021
@Podshot Podshot requested a review from gentlegiantJGC June 27, 2021 04:22

class ResultDialog(SimpleDialog):
def __init__(self, parent, block_location_info):
super(ResultDialog, self).__init__(parent, "Block Locations")
Copy link
Member

Choose a reason for hiding this comment

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

You can just do super().__init__(parent, "Block Locations")


class FindBlock(SimpleOperationPanel):
def __init__(self, parent, canvas, world, options_path):
SimpleOperationPanel.__init__(self, parent, canvas, world, options_path)
Copy link
Member

Choose a reason for hiding this comment

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

You can just do super().__init__(parent, canvas, world, options_path)

def _find_single_blockstate(self, world, dimension, selection):
target = world.block_palette.get_add_block(self._get_target_block())

iter_count = len(list(world.get_chunk_slice_box(dimension, selection, False)))
Copy link
Member

Choose a reason for hiding this comment

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

I know I have done this a bit but there is a quicker way. I should look into fixing the other places.
len(list(world.get_coord_box(dimension, selection, False)))
This should get the same length (or at least close) without having to pre-load all the chunks.

options.get("target_block_options", [])
or [world.level_wrapper.platform]
),
show_pick_block=True,
Copy link
Member

Choose a reason for hiding this comment

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

You haven't hooked up the pick block tool to the render.
Check out the fill operation for an example of how to do that.

@gentlegiantJGC
Copy link
Member

I like what you have done. I have commented on some minor code issues.
A few more issues.
The output x and z coordinates are wrong. It looks like they need the chunk base location added to them.
It would be really nice if you could click on one of the coordinates in the UI and get teleported to the location in the world. I don't think that should be too difficult to do.

Sorry about the next bit. It is a bit rambly but I don't know how to explain it concisely
One issue that would need quite a bit of rewriting is that you are doing the matching in the universal format. I don't have an issue with this but there are some cases where it might give some obscure results.
The better option would be to convert each blockstate from the universal format and see if it matches the block defined in the version format.

Example:
Block to find Bedrock 1.17 minecraft:oak_stairs[upside_down_bit=0b,weirdo_direction=0]
When converted to universal minecraft:oak_stairs[material=oak,half=bottom,facing=east,shape=straight]

Universal blocks in the world
minecraft:oak_stairs[material=oak,half=bottom,facing=east,shape=straight]
minecraft:oak_stairs[material=oak,half=bottom,facing=east,shape=inner_left]
minecraft:oak_stairs[material=oak,half=bottom,facing=east,shape=inner_right]

If you match in universal only the first will match. The blocks in world converted to Bedrock 1.17 will all be the same as the first blockstate.

@Podshot
Copy link
Member Author

Podshot commented Jun 28, 2021

For finding locations of a single blockstate I compare using the universal format, but for finding all the substates of a block I first convert everything to the versioned block, are you saying that I should do the same for searching when doing a single blockstate?

@gentlegiantJGC
Copy link
Member

gentlegiantJGC commented Jun 28, 2021

You should should be converting to the version that is chosen in the BlockDefine UI not the world version that way you are matching in whatever format the user defined the block in. Doing this will allow it to match any blocks that equate to the chosen block in the chosen version.

Sorry about closing the issue. I think I hit shift + enter on autopilot to do a new line like in discord which sent the message and closed the issue

Comment on lines 169 to 177
blocks_to_find = blocks_to_find.union(
filter(
lambda b: b[1].namespaced_name == versioned_block_name,
map(
lambda b: (b[0], block_translator.from_universal(b[1])[0]),
world.block_palette.items(),
),
),
)
Copy link
Member

Choose a reason for hiding this comment

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

This up here is fine but as each chunk is loaded if new blockstates are found in that chunk they will be added to the block palette.
In the loop below you should check if the palette size has changed and scan the new blocks

Comment on lines 187 to 189
for internal_id, block in blocks_to_find:
extend_func = results[block.full_blockstate].extend
for chunk, slices, _ in chunk_slices:
Copy link
Member

Choose a reason for hiding this comment

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

These loops would be better the other way around so that you are only loading each chunk once and doing all the work on that chunk in one go.

@gentlegiantJGC
Copy link
Member

It is also worth mentioning that I am working on a UI that will allow the user to pick one or more values for each property.
It would probably make sense for this to use that UI which will allow a lot more flexibility for the users.
Eg if the user wanted to find all upside down stair blocks regardless of their rotation

@Podshot Podshot marked this pull request as draft July 2, 2021 14:46
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants