Add options for building mesh/collision data into metadata of generated nodes

[the-argus]

These options can propagate data necessary for getting the texture on the part of a mesh corresponding to a collision.

Added as per discussion https://github.com/orgs/func-godot/discussions/59

Outstanding questions / known issues:

• I haven't tested this at runtime yet, so I'm sure there are some math issues. I'll get to that later this week, hopefully.
• I am uncertain if my method of finding average / center position for faces is correct. I'll find out when I test later, but please lmk if it is clearly wrong.
• The shape_index_to_faces_range_map provides redundant information for concave meshes, but currently it still gets built for them and then thrown away.
• I am passing scale factor to the surface gatherer so that it can do the map file -> godot translation in the loop. Feels like a bit of a hack but I wanted to avoid a second pass over anything.
• Since we address faces in the metadata by triangles, but "textures", "normals", and "positions" are the same for all triangles in the face of a brush, there can be quite a bit of duplication for those fields. If an index buffer was included, this would be fixed and appending vertex info would require no processing at all, as it could be copied straight from the result of fetch_surfaces.

On the example_basic scene, I rebuilt with all metadata enabled and file size went from 16KB to 20KB. I am going to test this out on more complex levels later, and also measure runtime memory usage. I don't think its a concern but I like having a frame of reference. When the docs get updated it would be nice to provide some estimates of changes in disk/memory usage when enabling this feature in different capacities.

[RhapsodyInGeek]

The shape_index_to_faces_range_map provides redundant information for concave meshes, but currently it still gets built for them and then thrown away.

Don't throw it away. It may seem redundant, but if someone is using a mix of concave and convex brushes it can be convenient to have them use the same methods regardless. Let the user decide if they want to throw it away in specific cases. Otherwise they may be asking for data and not knowing why they're not receiving it.

I am passing scale factor to the surface gatherer so that it can do the map file -> godot translation in the loop. Feels like a bit of a hack but I wanted to avoid a second pass over anything.

Instead of passing scale_factor what about passing map_settings? It might be helpful for future additions / changes (I'm thinking about patch and phong support).

When the docs get updated it would be nice to provide some estimates of changes in disk/memory usage when enabling this feature in different capacities.

We'll probably need a bigger tutorial (like what exists for "Why Not Worldspawn?" and "Runtime Building") on what it's actually intended for and how it all works. I don't think merely adding it to the SolidEntity reference will do.

I'll check this out some time tomorrow probably (trying not to check it out right now, it's 1AM) and give it a more thorough study.

[RhapsodyInGeek]

add_collision_shape_face_range_metadata is a more accurate name for the variable.

[RhapsodyInGeek]

Should be var metadata: Dictionary = func_godot.surface_gatherer.out_metadata to match style throughout func_godot_map.gd. Try to avoid := typing when possible, try to be explicit.

I know, Core needs a lot of cleanup in this area.

[RhapsodyInGeek]

collision_shape_to_face_range_map is more accurate.

[RhapsodyInGeek]

Don't throw it away. Some users may want this for some reason or another, use cases aren't limited to what we think of.

[the-argus]

How can I keep it? The concept of collision_shape_to_face_range_map doesnt make sense in this case, since each key corresponds to a single Vector2i, and there is only one possible key: the name of the single collision shape generated for this node.
could be solved by making each node name a key to an Array[Vector2i] instead... a bit odd since the arrays will always contain one item when building convex.

[RhapsodyInGeek]

I'm not sure why this doesn't make sense. I know that there'll only be one possible key in a concave collision entity, but the point is that the data is consistent between convex and concave. Devs may want or need unified methods between the two. Once again, even if it may not seem useful to you, it might be useful to someone else and there's no reason to change the behavior in this instance between convex and concave.

Don't overthink it or overdesign it too much. If you're already grabbing each collision shape and providing a face range for each, it doesn't matter if it's 10 shapes or 1: the output data should remain consistent. If someone wanted that data on a concave collision entity for some reason they have to manually choose to do so, and far be it from us to deny them that data for arbitrary reasons.

[the-argus]

Currently, the output for a concave collision shape is something along the lines of { "entity_#_collision_shape_0": Vector2i(0, num_faces) }. Is this acceptable? If so, I can avoid storing/building information about which brushes correspond to which faces (which is what I do in the case of convex).
When I say I am throwing the data away, I am talking about that brush -> faces mapping, which is internal and only really useful in the convex case. My thought was that maybe the user would still want that data when building concave collision. I was thinking pretty much what you are saying about not denying the user access to data that we already have. But yes it is a bit of overthinking since it would require a change in the format to provide it to them.

[RhapsodyInGeek]

As long as the output data the user receives is the same format between convex and concave, then skipping calculations when you already know the answer is perfectly acceptable (and preferred). The only 4 things that matter are Output Data, Input Method, Readability, and Performance, preferably in that order. If those are accomplished we're doing it right.

[the-argus]

Okay, I will skip the calculations for concave and otherwise keep things the same.

[RhapsodyInGeek]

collision_shape_to_face_range_map

[RhapsodyInGeek]

COLLISION_SHAPE_TO_FACE_RANGE_MAP

[RhapsodyInGeek]

I said I wasn't going to look it over yet but then I did.

One thing I didn't add comments on was the spots in SurfaceGatherer where you append values to the metadata arrays. Since we already know how large the arrays need to be thanks to the tri counts, we can resize them to what we need right from the start, then instead of appending values we can simply assign them to empty elements. Depending upon how many iterations an entity might parse, this can give a decent boost to performance by only allocating memory once instead of every iteration.

SurfaceGatherer is somewhere we need to try to squeeze as much performance as possible, since it might be where we need to implement Phong in the rework of it.

[the-argus]

Okay, after a bunch of testing I think this PR is functionally complete. I do feel like the surface gatherer run function is getting quite complex. It might be nicer to move stuff out of there into a new file with some class FuncGodotMetadataBuilder whose purpose is just to get instantiated during the surface gatherer run(). But also hey maybe its fine.

Depending upon how many iterations an entity might parse, this can give a decent boost to performance

I was curious about this, so I tried making a really big map (just cutting up a bunch of brushes to be really complex and then duplicating them everywhere) and putting a really basic benchmark in surface gatherer:

var start := Time.get_unix_time_from_system()
# ...
if not out_metadata["vertices"].is_empty(): # only when building metadata
  print(Time.get_unix_time_from_system() - start)

I had two versions of the script which I would switch between, where I would replace all instances of for i in num_tris: array.append(item) with:

var i = array.size()
array.resize(array.size() + num_tris)
for j in num_tris:
    array[i + j] = item

And the vertices one with a slightly modified version to account for 3x the indices.

I was only able to find differences of 2-3 milliseconds and it was not consistently in favor of one or the other. But while doing this I noticed that the average runtime of the whole build was 3.842 seconds and the average runtime of surface gatherer was 0.044 seconds (roughly 1% of build time). I don't think surface gatherer is the best target for optimization and could probably afford the hit from the phong rework EDIT: the number I gave for surface gatherer was average function runtime not total, which is more like 0.88 seconds, or 23% of runtime. Confused myself, disregard that.

Anyways: I don't think there's a significant benefit to preallocating the additions to the metadata arrays, and it's fine as is. At least, if the way I did it above is correct.

[RhapsodyInGeek]

At least, if the way I did it above is correct.

Honestly, hard to say without it being in context. The thing is when you append to Godot Arrays, it allocates more than just one additional element, partly for memory optimization and partly for performance of looped appending. It might be that you're not seeing significant gains because you're only allocating a handful of tris at once (IIRC num_tris refers to the number of triangles in the brush, not the whole brush entity?).

If it works while still keeping builds relatively quick, then it's fine for now. I may switch it over to preallocating depending upon how much perf I need to squeeze out of SurfaceGatherer for Phong (though I'm hoping to find a better method than just brute forcing it, but that's for another PR to solve).

I'll test this this week. Hopefully merged by the weekend.

[the-argus]

It might be that you're not seeing significant gains because you're only allocating a handful of tris at once (IIRC num_tris refers to the number of triangles in the brush, not the whole brush entity?)

It’s tris on the face, and it’s typically a number between 1 and 3. I was skeptical it would provide performance benefits for the reasons you mentioned but I didn’t want to say for sure since my frame of reference for what the data typically looks like was the example basic map.

Thinking about what might be more effective. Maybe the number of tris in all the faces of a brush can be easily calculated from the number of vertices in the whole brush or something along those lines? I’m not familiar with geo generator. If not, I think estimation would also be a really effective approach since brushes are often somewhere between a plane and a cube.

[RhapsodyInGeek]

I think it might be best to wait on figuring the preallocation out, as I suspect the answer may lead into the necessary rework for the Phong Phix, since the solution involves iterating through every vertex in a brush entity across all of their brushes to merge them and average out the normals.

We'll probably need to rethink the data structures created by GeoGenerator, too. I dunno. Best to worry about it all in another PR though.

[RhapsodyInGeek]

Did some testing, looks like we're still not getting the right face positions.

It's because you're not treating the triangles as the faces, but rather summing up all of the vertices in the BrushFace. Need a lot of clean up in how FuncGodot defines things, but this should be resolvable pretty easily. I'll comment with a fix in a bit.

[RhapsodyInGeek]

Figured it out.

Replace this section:

				var last_index: int = -1
				var avg_vertexpos := Vector3.ZERO
				var total_vertexpos: int = 0
				for i in range(num_tris * 3):
					surf.indicies.append(face_geo.indicies[i] + index_offset)
					if include_vertices_metadata:
						var vertex: Vector3 = surf.vertices[surf.indicies.back()].vertex
						vertices.append(Vector3(vertex.y, vertex.z, vertex.x) * map_settings.scale_factor)
					if include_positions_metadata:
						var index := face_geo.indicies[i] + index_offset
						# NOTE: this relies on new indices always being in ascending order
						if index > last_index:
							var vertex: Vector3 = surf.vertices[index].vertex
							avg_vertexpos += Vector3(vertex.y, vertex.z, vertex.x) * map_settings.scale_factor
							total_vertexpos += 1

				if include_positions_metadata:
					avg_vertexpos /= total_vertexpos
					for i in num_tris:
						positions.append(avg_vertexpos)

with this:

				var avg_vertex_pos := Vector3.ZERO
				var avg_vertex_pos_ct: int = 0
				for i in range(num_tris * 3):
					surf.indicies.append(face_geo.indicies[i] + index_offset)
					var vertex: Vector3 = surf.vertices[surf.indicies.back()].vertex
					vertex = Vector3(vertex.y, vertex.z, vertex.x) * map_settings.scale_factor
					if include_vertices_metadata:
						vertices.append(vertex)
					if include_positions_metadata:
						avg_vertex_pos_ct += 1
						avg_vertex_pos += vertex
						if avg_vertex_pos_ct == 3:
							avg_vertex_pos /= 3
							positions.append(avg_vertex_pos)
							avg_vertex_pos = Vector3.ZERO
							avg_vertex_pos_ct = 0

[the-argus]

I misunderstood what you intended by face average position in the discussion, then. I thought it was the center of the brush face not the induvidual tris. But what's the point of getting the average position of tris? I mean consider the difference in the case of getting the position for a known face index:

var verts: PackedVector3Array = get_meta(&"func_godot_mesh_data")["vertices"]
var position: Vector3 = (verts[face_index] + verts[face_index + 1] + verts[face_index + 2]) / 3
# vs
var position2: Vector3 = get_meta(&"func_godot_mesh_data")["positions"][face_index]

And the user can easily write a script to read the vertices array at build time and convert it into a triangle positions array, and delete the original vertices array if they need to save space. Then they can loop through that in an entity to find the nearest face if they need. Or, better, yet: replace positions with some face_vertex_ranges that lets you know what vertices are a part of the same brush face, enabling the user to build a positions array themselves in either the way I do or the triangle positions way you are saying here. Although I ought to move this back to the discussions page where we talked about format at this point, heh.

[RhapsodyInGeek]

I misunderstood what you intended by face average position in the discussion, then. I thought it was the center of the brush face not the induvidual tris.

When a map is imported via Godot, brushes don't really exist as a concept. A Brush Face does not equal a Mesh Face, and Godot only really cares about mesh faces. Brushes don't exist in Godot. (they don't even really exist in Quake or Half-Life after BSP compile, but that's a whole other philosophical debate...)

But what's the point of getting the average position of tris?

Just because you don't see the point doesn't mean someone else won't find a use. If you give it a bit of thought, you can probably extrapolate on more advanced use cases just from this screenshot:

The feature isn't just about getting textures from faces for step sounds and decals. It's convenient access to the mesh data as a whole, and other devs will come up with use cases that we never even imagined with that data just because it's there.

[the-argus]

When a map is imported via Godot, brushes don't really exist as a concept. A Brush Face does not equal a Mesh Face

My thought here is that someone may wish that were not the case, and try to reverse-engineer what faces belonged to what brushes in the level editor. That's information which we do not propagate to godot, the same way that without this PR we are not propagating what mesh faces corresponded to what collision shapes, since that concept also doesn't exist in godot.

Just because you don't see the point doesn't mean someone else won't find a use.

But I mean- that face position information is already available with a bit of math on the vertices array, regardless of what use cases exist now or will be discovered in the future. I ask what the point of providing it in an additionally, slightly different format is.

[RhapsodyInGeek]

My thought here is that someone may wish that were not the case, and try to reverse-engineer what faces belonged to what brushes in the level editor

Brushes don't exist in Godot. Brush faces don't correspond to actual faces in the mesh. It really is as simple as that. While we could add a Brush Face positions, we'd also be looking at brush face vertices, but there isn't a clean flexible way to support faces that have wildly varying vertex counts other than Array[PackedVector3Array] but now we're making the data extremely unwieldy. Also, while the brush face has to be convex, that doesn't mean the center position will land on the brush face. It all starts becoming extremely inconsistent.

It also betrays one of the main concepts behind mapping with FuncGodot: you aren't making a map of brushes, you are making a map that will be turned into meshes and therefore need to think in terms of meshes, not brushes.

I'm not outright shutting down any implementations of adding Brush Face data, but I'd need some more convincing that it's worth adding on top of what we already grab, and to have more of a discussion on the best way to go about it and what else we need to get. It'd have to be in another PR though, this one should remain focused on the actual mesh triangle data.

... the same way that without this PR we are not propagating what mesh faces corresponded to what collision shapes, since that concept also doesn't exist in godot.

That's different because both Mesh Instances and Collision Shapes exist in Godot, and there is often a match or at least some relationship between the collision and the visuals. Brushes are just an abstract data set via FuncGodot, but have to be heavily modified in order to become usable Mesh and Shape resources.

But I mean- that face position information is already available with a bit of math on the vertices array, regardless of what use cases exist now or will be discovered in the future. I ask what the point of providing it in an additionally, slightly different format is.

You're overthinking it too much. We can provide that info easily, efficiently, and conveniently without any trouble while also having it line up with the other data we pull.

All of the other data operates under the assumption of triangles, since that's how Godot understands mesh data. Even the collision_shapes_to_face_ranges operates under the assumption of tris, not brush faces / planes.

In any case, I've tested it with my fix, it all works and works well, so I can merge it as soon as you apply the fix. I'd like to get this PR merged so we can get a 2024.3 out and finally fix the Asset Lib version. It'll also be helpful to have 2024.3 ready for the tutorials I plan on making.

[RhapsodyInGeek]

@the-argus If you can apply this change #60 (comment) to get the mesh face positions, I can merge this after one last test.

I'd offer to do it myself, but I can't contribute to this branch.

With this implemented, I can add it to the docs and put out a 2024.3 release. It'll also let me start looking at bigger changes to the build processes to try to implement proper phong support.

[the-argus]

🫡
I look forward to the tutorials! And the asset lib working properly will be great.