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

Allow importing 3D models without colliders or with KinematicBody/Area #39924

Closed

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jun 28, 2020

EDIT: Marked as draft due to the below discussion. We can make a better solution than what this PR alone offers.

Implements and closes godotengine/godot-proposals#1029

This PR adds an enum for how colliders should be imported. Right now the possible values are None, Static, Rigid, and Area. Static is the default value and has the same behavior as the current master.

This was tested by copying core.glb from the TPS demo into a mostly empty project.

@aaronfranke
Copy link
Member Author

Rebased and reworked following #47166.

@reduz
Copy link
Member

reduz commented Mar 23, 2021

RigidBody makes sense because you can put a barrel in there or some props that the player can stumble into and which will have physics. KinematicBody does not make a lot of sense unless you want to do moving platform or something and this object is animated, in which case it could be added. That said, the idea is to remove KinematicBody in master so I guess this PR does not make much sense based on this.

@aaronfranke aaronfranke force-pushed the 3d-model-colliders branch 2 times, most recently from 2a52153 to 50a48a2 Compare March 23, 2021 17:23
@aaronfranke
Copy link
Member Author

@reduz I made the requested changes, as such this PR now expects that KinematicBody will be removed.

@aaronfranke aaronfranke force-pushed the 3d-model-colliders branch 2 times, most recently from d320fce to 64f9ed0 Compare May 5, 2021 02:36
@aaronfranke aaronfranke force-pushed the 3d-model-colliders branch from 64f9ed0 to 399cfae Compare May 8, 2021 06:22
@aaronfranke aaronfranke force-pushed the 3d-model-colliders branch 2 times, most recently from 3bd22be to e5dbb28 Compare June 3, 2021 23:28
@aaronfranke aaronfranke force-pushed the 3d-model-colliders branch 2 times, most recently from f66bc69 to aa9a7bf Compare June 18, 2021 04:23
@pouleyKetchoupp
Copy link
Contributor

pouleyKetchoupp commented Oct 4, 2021

Unless I'm missing something, this PR looks like it's superseded by #51985 (#52266), which already adds the possibility for different body types or no physics at all at import.
Is there anything else this PR does that's not implemented in the current import process?

@aaronfranke
Copy link
Member Author

@pouleyKetchoupp No, these are different things. This PR is about changing the existing colliders that Godot generates for meshes marked as colliders in the file. #52266 is about allowing any mesh to become a collider, on an individual basis.

Here are two screenshots, both taken in the current master branch. The model is core.glb from the TPS demo.

core1

These StaticBody nodes currently can't be changed to any other type. There is only one option to skip import, but this has to be manually applied to every single one. This PR allows users to change all of them to any type, or skip all of them with one button. Note that there is no need to have overrides for individual ones, because if granularity is required, you can use what #52266 added:

core2

These are meshes. #52266 allows importing them as colliders, and users can leave disabled for no colliders, or pick Static, Dynamic, or Area (note: missing Animatable).

@pouleyKetchoupp
Copy link
Contributor

@aaronfranke Thanks for the explanation! This PR makes sense in this case, but I wonder if for the long term we could find a more similar process between the two cases, rather than different systems that generate similar nodes in the end.

Maybe something like:

  • Option on the individual mesh nodes to enable/disable physics (which would show/hide the physics node in the tree)
  • Option on the physics node to switch the body type
  • Options on the collider itself for concave, convex decomposition, primitive types.

So all the different options from #52266 could also apply for colliders import.

@aaronfranke
Copy link
Member Author

@pouleyKetchoupp Indeed, it would be good to unify the systems. Another thing I noticed is that with the "old" system that this PR modifies, the physics objects are created as children of the meshes (which is weird), while the option added in #52266 makes it so that the meshes are children of the physics objects (which makes sense).

In your bulleted list most of that is possible with #52266 except that there are no physics nodes shown in the preview tree when changing the option added in #52266 (the only physics nodes in the preview tree are from the "old" system), and instead of the "concave, convex decomposition, primitive types" setting being on the collider itself it's on the mesh.

There is also the matter of what to do with the information from 3D models that marks specific meshes as colliders.

I'm thinking something like this (this is more of a plan of action while your list is a summary of what's desired):

  • Do what you suggest with showing physics nodes in the preview tree so that it represents the scene better.

  • Delete the "old" system that this PR modifies, and have everything handled by the 52266 system.

  • Add a ColliderImportMode setting like in this PR, but instead of a separate system, it controls the default setting for the 52266 system for meshes that are marked as colliders (meshes not marked as colliders always default to no physics).

@aaronfranke
Copy link
Member Author

Closing because this isn't good as-is. However, the scene import workflow should still be improved once an approach is agreed on, and the discussion contained within this thread is still very useful. I'll mark this as salvageable for now, but a lot of this would need to be rewritten in a different way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow importing 3D models without colliders
4 participants