Allow the user to override the vertex semantics. by pmuetschard · Pull Request #1847 · google/gapid

pmuetschard · 2018-05-01T19:47:09Z

Adds a new settings-gear button to the geo view, that when clicked, shows a dialog where the user can assign the sementic type to each vertex stream of the current model.

Fixes #960

ben-clayton · 2018-05-02T12:31:48Z

gapis/service/path/path.proto

-    bool faceted = 1; // If true then normals are calculated from each face.
+    // If true then normals are calculated from each face.
+    bool faceted = 1;
+    // If true, the actual data will be missing from the vertex.Buffers.


Maybe include a/the usage case for this?

ben-clayton · 2018-05-02T12:32:49Z

gapis/service/path/path.proto

+    bool exclude_data = 2;
+    // Hints that override the semantic guessing. Maps vertex attribute names to
+    // their intentended semantic type.
+    map<string, vertex.Semantic.Type> vertex_semantics = 3;


We've intentionally avoided proto's map to date. While I am happy to revisit that decision, I'd prefer consistency for now.

Do you think that name to type is the right association here?
I was thinking that stream index to type might be more explicit and less error prone (like when there is no name).

No more maps.... though, why?

There always seems to be a name and it is the most descriptive thing available, so it makes sense to me. It doesn't relay on the order, which does not appear to be stable.

ben-clayton · 2018-05-02T12:36:04Z

gapis/api/gles/draw_call_mesh.go

-	dci, err := dc.getIndices(ctx, c, s)
-	if err != nil {
-		return nil, err
+	noData := p.Options != nil && p.Options.ExcludeData


Possibly the same as p.GetOptions().GetExcludeData() ?

ben-clayton · 2018-05-02T12:37:10Z

gapis/api/gles/draw_call_mesh.go

+
+	var dci drawCallIndices
+	if noData {
+		dci = drawCallIndices{nil, dc.getDrawMode(), false}


If we're not getting data, is the draw mode important? If not, can we drop the new interface method?

It's not currently important, but I think it makes sense to keep it, since it's part of the mesh metadata.

ben-clayton · 2018-05-02T12:39:20Z

gapis/api/gles/guess_semantics.go

 	}
 }
+
+func needsGuess(s *vertex.Stream) bool {


Isn't this already covered with the taken map?

No. If a hint is provided, I don't want the guessing to override the hint.

ben-clayton · 2018-05-02T12:40:05Z

gapis/api/vulkan/draw_call_mesh.go


 	stats := &api.Mesh_Stats{}

+	noData := p.Options != nil && p.Options.ExcludeData


ben-clayton · 2018-05-02T12:41:35Z

gapis/api/vulkan/draw_call_mesh.go

 	taken := map[vertex.Semantic_Type]bool{}
+	if len(hints) > 0 {
+		for _, s := range vb.Streams {
+			if t, ok := hints[s.Name]; ok && !taken[t] {


@AWoloszyn do streams typically have names in vulkan?

They do, though they are not "names" as such (yet), but are of the form binding=##, location=##.

Fetching the mesh without data allows clients to query the mesh metadata before deciding how to fetch the data. The semantic hints allow overriding of the not-so-state-of-the-art semantic AI guesser.

Adds a new settings-gear button to the geo view, that when clicked, shows a dialog where the user can assign the sementic type to each vertex stream of the current model.

pmuetschard

Done. PTAL.

pmuetschard · 2018-05-02T17:16:47Z

gapis/api/gles/draw_call_mesh.go

-	dci, err := dc.getIndices(ctx, c, s)
-	if err != nil {
-		return nil, err
+	noData := p.Options != nil && p.Options.ExcludeData


pmuetschard · 2018-05-02T17:17:44Z

gapis/api/gles/guess_semantics.go

 	}
 }
+
+func needsGuess(s *vertex.Stream) bool {


No. If a hint is provided, I don't want the guessing to override the hint.

pmuetschard · 2018-05-02T17:17:50Z

gapis/api/vulkan/draw_call_mesh.go


 	stats := &api.Mesh_Stats{}

+	noData := p.Options != nil && p.Options.ExcludeData


pmuetschard · 2018-05-02T17:19:43Z

gapis/api/vulkan/draw_call_mesh.go

 	taken := map[vertex.Semantic_Type]bool{}
+	if len(hints) > 0 {
+		for _, s := range vb.Streams {
+			if t, ok := hints[s.Name]; ok && !taken[t] {


They do, though they are not "names" as such (yet), but are of the form binding=##, location=##.

pmuetschard · 2018-05-02T17:19:53Z

gapis/service/path/path.proto

-    bool faceted = 1; // If true then normals are calculated from each face.
+    // If true then normals are calculated from each face.
+    bool faceted = 1;
+    // If true, the actual data will be missing from the vertex.Buffers.


pmuetschard · 2018-05-02T17:33:01Z

gapis/service/path/path.proto

+    bool exclude_data = 2;
+    // Hints that override the semantic guessing. Maps vertex attribute names to
+    // their intentended semantic type.
+    map<string, vertex.Semantic.Type> vertex_semantics = 3;


No more maps.... though, why?

There always seems to be a name and it is the most descriptive thing available, so it makes sense to me. It doesn't relay on the order, which does not appear to be stable.

pmuetschard · 2018-05-02T17:36:13Z

gapis/api/gles/draw_call_mesh.go

+
+	var dci drawCallIndices
+	if noData {
+		dci = drawCallIndices{nil, dc.getDrawMode(), false}


It's not currently important, but I think it makes sense to keep it, since it's part of the mesh metadata.

pmuetschard requested review from AWoloszyn and ben-clayton May 1, 2018 19:47

ben-clayton reviewed May 2, 2018

View reviewed changes

pmuetschard added 3 commits May 2, 2018 10:09

Fix various warnings in the client.

ea61545

Allow fetching of a Mesh without any data and semantic hints.

5c97b96

Fetching the mesh without data allows clients to query the mesh metadata before deciding how to fetch the data. The semantic hints allow overriding of the not-so-state-of-the-art semantic AI guesser.

Allow the user to override the vertex semantics.

156ee84

Adds a new settings-gear button to the geo view, that when clicked, shows a dialog where the user can assign the sementic type to each vertex stream of the current model.

pmuetschard force-pushed the geo branch from 90a11d7 to 156ee84 Compare May 2, 2018 18:33

pmuetschard commented May 2, 2018

View reviewed changes

ben-clayton approved these changes May 2, 2018

View reviewed changes

pmuetschard merged commit ddae44f into google:master May 2, 2018

pmuetschard deleted the geo branch May 2, 2018 18:52


		stats := &api.Mesh_Stats{}

		noData := p.Options != nil && p.Options.ExcludeData

Comments

Conversation

pmuetschard commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmuetschard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants