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

Automatic navmesh generation support using recast #213

Merged
merged 17 commits into from
Jun 20, 2023
Merged

Automatic navmesh generation support using recast #213

merged 17 commits into from
Jun 20, 2023

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Mar 29, 2023

Automatic navigation mesh generation support using recast. Based on the work from https://github.com/przemir/RecastBlenderAddon

@keianhzo keianhzo added this to the 1.2.0 milestone Mar 29, 2023
@keianhzo keianhzo self-assigned this Mar 30, 2023
@Exairnous Exairnous self-requested a review April 24, 2023 07:35
Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

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

This is going to be so nice! A couple things though.

  • A visible component with visibility unchecked should be added along with the navmesh component.
  • The max slope property should display degrees instead of radians (subtype='ANGLE' in the float property).
  • I think what's in Spoke should be matched as closely as possible since there isn't any documentation and people are already familiar with what's in Spoke:
    • I think the only significant differences in property names/order are that the max step height is named max climb, and that max climb and max slope are swapped.
    • Some of the default values also don't match what's in Spoke, so I'll list the Spoke defaults here (defaults taken from: https://github.com/mozilla/Spoke/blob/master/src/editor/recast/recast.worker.js):
      • cell size: 0.166
      • cell height: 0.10
      • agent height: 1.70
      • agent radius: 0.5
      • maximum step: height 0.3
      • maximum slope: 45 degrees (0.785398 radians)
      • minimum region area: 1.00m2 (adding unit='AREA' in the float property makes it display m2 after the number)
      • region merge size: 20
      • edge max length: 12
      • edge max error: 1
      • verts per poly: 3
      • detail sample distance: 13
      • detail max sample error: 1
    • I think it would also be a good idea to hide the more advanced options by default, either by a checkbox or subpanel, so as not to confuse people. The hidden options would be:
      • region merge size
      • edge max length
      • edge max error
      • verts per poly
      • detail sample distance
      • detail max sample error
    • It would also be nice to offer the auto cell size option. I think this should work okay (patterned after the code in https://github.com/mozilla/Spoke/blob/master/src/editor/nodes/FloorPlanNode.js):
def get_auto_cell_size():
    bounding_boxes = []
    for obj in bpy.context.selected_objects:
        if obj.type == 'MESH':
            bbox = [obj.matrix_world @ Vector(point) for point in obj.bound_box]
            bounding_boxes.extend(bbox)

    bound_box_x_coords = []
    bound_box_y_coords = []
    for point in bounding_boxes:
        bound_box_x_coords.append(point.x)
        bound_box_y_coords.append(point.y)
        
    size_x = abs(min(bound_box_x_coords) - max(bound_box_x_coords))
    size_y = abs(min(bound_box_y_coords) - max(bound_box_y_coords))
    area = size_x * size_y
    
    return pow(area, 1 / 3) / 50
  • It might also be nice to add a reset to defaults button (since you can't just delete a floor plan element and re-add it to get the defaults back)

addons/io_hubs_addon/components/utils.py Outdated Show resolved Hide resolved
Comment on lines 20 to 22
import bpy

import bpy
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate import bpy

addons/io_hubs_addon/third_party/recast.py Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
@Exairnous
Copy link
Contributor

Got some feedback from people at Taco Tuesday Testing Day on this patch and it was requested that the navmesh not be selected after generation and that the original selection be maintained to allow for easier tweaking.

@Exairnous
Copy link
Contributor

I've been thinking more about adding the visible component as well as the navmesh component and it would make sense to add the visible component as a dependency of the navmesh component, except that when the visible component got added it would be added with it's default of visible set to true, while the navmesh should have visible set to false. So this got me to thinking that it might be nice to be able to override the default property values of dependencies. Obviously this doesn't need to be addressed in this PR, but do you think setting dependency properties is something we should add?

@keianhzo
Copy link
Contributor Author

We can maybe override the HubsComponent init method to initialize dependencies.

@Exairnous
Copy link
Contributor

Yes, as we talked about in the add-on meeting, we'll need to rearrange the order init is called, but then that's a good place for it.

Jack Saturn and I found a bug when testing one day, it appears to error out generating the navmesh when certain non-mesh objects are present in a scene, e.g. reflection probes. Putting the non-mesh objects in a separate collection and disabling it will work around this bug, but it would be ideal if they can just be skipped.

@keianhzo
Copy link
Contributor Author

keianhzo commented Jun 5, 2023

@Exairnous I think I've address most of the feedback if you want to review again before I merge

@Exairnous
Copy link
Contributor

Exairnous commented Jun 6, 2023

Thanks.
Yes, will review :)

Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

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

I found a few things which I've noted in inline comments, but this is looking really good.

Also, is there a reason you made your own subpanel for the advanced settings, rather than using a blender one via defining another panel class and adding bl_parent_id = "SCENE_PT_blendcast" and bl_options = {'DEFAULT_CLOSED'} (if you agree with having it closed by default)? I think it would fit in better stylistically here.

addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Show resolved Hide resolved
addons/io_hubs_addon/third_party/recast.py Outdated Show resolved Hide resolved
@keianhzo
Copy link
Contributor Author

keianhzo commented Jun 7, 2023

@Exairnous Thanks for all the feedback! Addressed.

Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

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

I added a commit to fix a couple small things, but other than that I think everything looks good.

@keianhzo keianhzo merged commit 5124328 into master Jun 20, 2023
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