Various brush entity improvements and fixes by EIREXE · Pull Request #24 · codecat/godot-tbloader

EIREXE · 2022-08-28T22:20:53Z

This includes:

Generate one mesh instance per entity, instead of one mesh instance per texture, this seems like a much more sensible way than using one meshinstance per texture
Fix the rotation of brush entities, previously the transform was applied on top, however this is wrong as the brush entity's geometry is already in godot space (and brush entities have no rotation anyways).
Always generate visuals for brush entities, this is because a brush entity doesn't have to make use of collision at all
Generate brush entity collisions as siblings of the mesh instance if the brush entity is of a physics type, this is so the parent is the one that handles all the physics of the brush entity

codecat · 2022-08-28T23:11:49Z

Thanks for the PR! I'll take a closer look at the code hopefully tomorrow.

a brush entity doesn't have to make use of collision at all

Right, but the opposite is also correct; not all brushes need visual but do need collision, for example invisible walls. Would that still be possible with this change?

EIREXE · 2022-08-28T23:15:16Z

Thanks for the PR! I'll take a closer look at the code hopefully tomorrow.

a brush entity doesn't have to make use of collision at all

Right, but the opposite is also correct; not all brushes need visual but do need collision, for example invisible walls. Would that still be possible with this change?

Invisible walls IMO should be done with a special clip texture like quake/source does that is configured in the TBLoader node, I have such thing implemented already in my own branch but it's not in this PR, my plan was to add it together with nodraw textures for skipping faces as a separate PR.

codecat · 2022-08-29T08:44:13Z

Most of the code looks good! 🎉

One thing I don't understand though is why are you passing ColliderType::None to build_entity_mesh and then calling add_collider_from_mesh later? That should already happen correctly in build_entity_mesh, shouldn't it?

I also have some minor style nitpicks if you wouldn't mind changing them 😊

codecat · 2022-08-29T08:45:42Z

Invisible walls IMO should be done with a special clip texture like quake/source does that is configured in the TBLoader node

I agree, but I think it should work both ways too. Perhaps this is not a problem though, particularly because I also want to allow customizing whether visual/collision is enabled through entity properties. (But that's for a separate issue/commit)

EIREXE · 2022-08-29T21:56:30Z

One thing I don't understand though is why are you passing ColliderType::None to build_entity_mesh and then calling add_collider_from_mesh later? That should already happen correctly in build_entity_mesh, shouldn't it?

Because the idea is that in the case of brush entities collision shapes should always be children of the entity instead of children of the mesh as is the case in static mode.

I also have some minor style nitpicks if you wouldn't mind changing them blush

Should be OK now

codecat · 2022-08-31T11:17:31Z

Because the idea is that in the case of brush entities collision shapes should always be children of the entity instead of children of the mesh as is the case in static mode.

This is why, when passing ColliderType::Mesh to build_entity_mesh, it adds the collider using add_collider_from_mesh to the specified parent. I think it would do the same if you pass a mesh collider type using need_collider = ColliderType::Mesh;, similarly to how it is done for RigidDynamicBody3D.

EIREXE · 2022-08-31T17:03:57Z

Because the idea is that in the case of brush entities collision shapes should always be children of the entity instead of children of the mesh as is the case in static mode.

This is why, when passing ColliderType::Mesh to build_entity_mesh, it adds the collider using add_collider_from_mesh to the specified parent. I think it would do the same if you pass a mesh collider type using need_collider = ColliderType::Mesh;, similarly to how it is done for RigidDynamicBody3D.

Hmm yeah you are right, I've gone ahead and done that

codecat · 2022-08-31T17:05:46Z

-			add_collider_from_mesh(node, mesh, need_collider_shape);
-		}
-	}
+	MeshInstance3D* mesh_instance = build_entity_mesh(idx, ent, node, ColliderType::Mesh, need_collider_shape);


I think you want to use need_collider here?

True, I'm dumb

This includes: - Generate one mesh instance per entity, instead of one mesh instance per texture, this seems like a much more sensible way than using one meshinstance per texture - Fix the rotation of brush entities, previously the transform was applied on top, however this is wrong as the brush entity's geometry is already in godot space (and brush entities have no rotation anyways). - Always generate visuals for brush entities, this is because a brush entity doesn't have to make use of collision at all - Generate brush entity collisions as siblings of the mesh instance if the brush entity is of a physics type, this is so the parent is the one that handles all the physics of the brush entity

codecat · 2022-08-31T17:19:34Z

+	// Create instance name based on entity idx
+	String instance_name = String("entity_{0}_geometry").format(Array::make(idx));


Doing this loses the entity name given in Trenchbroom. Do we really need that?

Actually, this might only lose the texture name, so it probably doesn't matter that much.

Yeah the point of that is that since we are now grouping textures into one mesh its a better idea to just use the entity id

codecat · 2022-08-31T17:29:16Z

Thank you! 🎉

EIREXE changed the title ~~Various brush entity improvements~~ Various brush entity improvements and fixes Aug 28, 2022

EIREXE force-pushed the brush_entities_fixes branch from 97e5ffa to 7dfcf3a Compare August 29, 2022 21:56

EIREXE force-pushed the brush_entities_fixes branch from 7dfcf3a to 9a99aae Compare August 31, 2022 17:03

codecat reviewed Aug 31, 2022

View reviewed changes

EIREXE force-pushed the brush_entities_fixes branch from 9a99aae to 3b6d7ca Compare August 31, 2022 17:08

codecat reviewed Aug 31, 2022

View reviewed changes

codecat merged commit a5cdd77 into codecat:master Aug 31, 2022

		// Create instance name based on entity idx
		String instance_name = String("entity_{0}_geometry").format(Array::make(idx));

Uh oh!

Conversation

EIREXE commented Aug 28, 2022

Uh oh!

codecat commented Aug 28, 2022

Uh oh!

EIREXE commented Aug 28, 2022

Uh oh!

codecat commented Aug 29, 2022

Uh oh!

codecat commented Aug 29, 2022

Uh oh!

EIREXE commented Aug 29, 2022

Uh oh!

codecat commented Aug 31, 2022

Uh oh!

EIREXE commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecat Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

EIREXE Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

codecat Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

codecat Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

EIREXE Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

codecat commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EIREXE commented Aug 31, 2022 •

edited

Loading