Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

DRAFT: broadcast a dynamically-generated mesh #396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ForrestTrepte
Copy link

This is code that we used in a particular project that I'm sharing here for reference. There might be aspects that should be more generalized before this is made a standard SpectatorView feature, but I don't have time to develop it further.

Potential issues include:

  • Hasn't been tested with the latest code in this repository.
  • I believe there are warnings about dynamic meshes elsewhere in the system that should be removed once dynamic meshes are supported.
  • Should have a test case or example content demonstrating that it works.
  • Contains an unnecessary CheckValue and logging that should probably be removed.
  • Transmits a dynamically-generated mesh, but doesn't detect dynamic changes to that mesh. Optionally, dynamically-changing meshes could be supported with a checksum of the mesh data.

@@ -27,11 +31,42 @@ protected override void WriteRenderer(BinaryWriter message, byte changeType)
if (HasFlag(changeType, MeshFilterChangeType.Mesh))
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested whether this flag gets set when someone modifies the meshFilter.mesh on update/start?

Copy link
Author

Choose a reason for hiding this comment

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

No. I have verified that this worked with dynamically-generated meshes in my project, but I didn't take a close look at exactly how the dynamic meshes were created.

I haven't tested changing a mesh after it has already been broadcast and I expect that changes after broadcast would not be detected.

{
ComponentExtensions.EnsureComponent<MeshRenderer>(gameObject);
MeshFilter filter = ComponentExtensions.EnsureComponent<MeshFilter>(gameObject);
filter.sharedMesh = mesh;
Copy link
Contributor

Choose a reason for hiding this comment

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

This may need to be filter.mesh. It looks like filter.sharedMesh isn't suggested for writing meshes.

Copy link
Author

Choose a reason for hiding this comment

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

I find Unity's MeshFilter.mesh/sharedMesh confusing, so I'm not sure. I think the warning about writing is if you modify a mesh using filter.sharedMesh. For example, if you do filter.sharedMesh.vertices = new Vector3[] then you could be inadvertently modifying a mesh that is shared by other objects. In this case, we are not modifying the existing sharedMesh but instead are first creating a Mesh and then assigning that mesh to filter.sharedMesh.

Not sure if I'm making much sense here, but I'm not sure what the right way is to do this or if it even matters which property is used in this case. I expect that assigning to filter.mesh would also be fine.

@chrisfromwork
Copy link
Contributor

Thanks for putting this review together. In general this seems like a great starting point. I think there need to be some small changes to reduce the logging for non error states and for cleaning up some todos. I did have a few questions:

  1. How many dynamic meshes did you include in a scene when filming? what were their triangle counts?
  2. Have you tried any meshes that were dynamically altered/not constructed on start?

@ForrestTrepte
Copy link
Author

@chrisfromwork in answer to your specific questions:

  1. 22 meshes, about half didn't actually have data (0 triangles in mesh), the remaining meshes had about 40 triangles or about 500 triangles. One of the meshes had 2300 triangles.
  2. No, all the meshes I tried were constructed during startup.

@chrisfromwork
Copy link
Contributor

Thanks for this information. Im going to do some more work on my end to test things out, but ideally we will be able to get this integrated into the repo

@ForrestTrepte
Copy link
Author

Thanks for doing the extra legwork to test and get this ready for integration, @chrisfromwork!

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

Successfully merging this pull request may close these issues.

2 participants