-
Notifications
You must be signed in to change notification settings - Fork 259
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
Custom Navigation Meshes #1077
Custom Navigation Meshes #1077
Conversation
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.
Made some minor suggestions and discussed some possible future enhancements, but lgtm
src/editor/nodes/FloorPlanNode.js
Outdated
})() | ||
); | ||
} else { | ||
node._navMeshSrc = navMeshSrc; |
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.
You might consider just assigning this above right after navMeshMode like:
node._navMeshSrc = navMeshSrc || ""
to get rid of the need for the nested if, and make the default value more explicit.
src/editor/nodes/FloorPlanNode.js
Outdated
if (navMeshMode === NavMeshMode.Custom) { | ||
loadAsync( | ||
(async () => { | ||
await node.load(navMeshSrc, onError); |
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.
Do we need the wrapping function? I think loadAsync(node.load(navMeshSrc, onError))
should do the same thing assuming loadAsync
just expects a promise.
this.setNavMesh(new Mesh(geometry, new MeshBasicMaterial({ color: 0x0000ff, transparent: true, opacity: 0.2 }))); | ||
|
||
if (this.navMesh) { | ||
this.navMesh.visible = this.editor.selected.indexOf(this) !== -1; |
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 needed? Guessing since you are emitting selectionChanged
at the end of this function the state should already get updated.
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.
Yeah, it looks like onSelect()
isn't called in response to the selectionChanged
event 🤷♂️
</> | ||
) : ( | ||
<InputGroup name="Custom Navmesh Url"> | ||
<ModelInput value={node.navMeshSrc} onChange={this.onChangeNavMeshSrc} /> |
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.
Not new to this PR, just anything taking a model in general so not something we need to address in this PR, but we might want to think about how we could visually indicate somehow that this field can have models dragged into it from assets. Ideally there might even be a little button on the field that pops up an asset picker (similar to how it works in Unity)
You can now specify a model for a custom navigation mesh. In the Floor Plan element's properties panel change the floor plan mode to "Custom" and drag and drop a model into the "Custom Navmesh Url" field.
Fixes #1075