Skip to content

Conversation

@podlipensky
Copy link
Contributor

Adds support of undefined (None) number of vertices/faces/colors in the mesh.

How to test:

  • Run updated unit tests.
  • Modify mesh_demo.py to provide different number of points at each run:
# Add extra vertex (1,2,3) to the mesh.
vertices_2 = np.concatenate((vertices, [[1, 2, 3]]))  
vertices_2 = np.expand_dims(vertices_2, 0)
...
for i in range(_MAX_STEPS):
    ver = vertices if i % 2 == 0 else vertices_2
    summary = sess.run(meshes_summary, feed_dict={
        vertices_tensor: ver,
        faces_tensor: faces,
        colors_tensor: colors,
        step: i,
    })

Run demo app and then launch tensorboard - observe mesh being rendered.

Also this PR changes the way we load data - from now on, every step is considered as unique data point and should be requested separately from the server (similar to image plugin). Please note that each mesh/step will produce 1 to 3 requests based on what data was passed into the summary (i.e. if there are no colors specified, then it will request only vertices and faces).

This PR depends on some TF-Graphics changes (I'll update it with new SHA once changes merged).

Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

About the JavaScript reviews: I can send you a pull request if you think that helps.

@stephanwlee stephanwlee requested a review from wchargin June 21, 2019 15:53
@stephanwlee
Copy link
Contributor

+reviewer: wchargin for Python readability.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

review for python

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Requesting changes to maintain data compatibility.

@podlipensky
Copy link
Contributor Author

FYI, updated SHA for TF Graphics-hosted files, so you can test the whole thing if needed. Please let me know if there is anything else is needed to merge the change.

Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

FE and most changes LGTM. I will let William finish review the Python part.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants