Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebGLRenderer: Introduce WebGLInfo #13404

Merged
merged 3 commits into from
Feb 25, 2018
Merged

WebGLRenderer: Introduce WebGLInfo #13404

merged 3 commits into from
Feb 25, 2018

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 22, 2018

This PR does two things:

  • It removes vertices from info.render since it does not work correctly with indexed geometries, see Remove vertices render info #13316. Instead info.render now contains the rendered primitives (faces, points and segments).
  • It refactors all info related code in WebGLRenderer by creating WebGLInfo.

@WestLangley
Copy link
Collaborator

It removes vertices from info.render since it does not work correctly with indexed geometries,

Actually, the reason for removing vertices is the data-structure of the geometry is irrelevant. What is relevant is the number of primitives rendered.

@WestLangley
Copy link
Collaborator

Also, the nomenclature segments is suggested because lines can be confusing in this case.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2018

What is relevant is the number of primitives rendered.

Yes, absolutely right 😅. But after all we avoid a source of confusion by removing vertices...

break;

default:
console.error( 'THREE.WebGLInfo: Unknown draw mode:', mode );
Copy link
Collaborator

@takahirox takahirox Feb 22, 2018

Choose a reason for hiding this comment

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

If I'm right, this console error message shows up each render call if user sets a wrong mode. That means the tons of the same error message will be displayed on user's console until user corrects the mode.

Any solutions to avoid that? Just ignoring wrong mode would be one of the options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, we've ignored it before so we can of course remove the default block. On the other hand, rendering with a invalid primitive is something that should actually never happen. I think it wouldn't be too bad if we produce error messages in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, right. I've read through WebGLRenderer and it seems that a wrong mode won't be set even user sets a wrong drawMode to Mesh. So I think it's ok to leave this console.error now.

@takahirox
Copy link
Collaborator

FYI @mrdoob, conflicts with #13144, which one are you gonna merge first?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2018

@takahirox I've just ported your enhancement for TRIANGLE_STRIP and TRIANGLE_FAN. It's probably easier to merge this one first and then #13144.

@takahirox
Copy link
Collaborator

takahirox commented Feb 22, 2018

@Mugen87 Cool. Then I'll remove the change of WebGL(Indexed)BufferRenderer from #13144 later. And considering we could use info of renderer from Editor... I'd try.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2018

This PR also "solves" the following issue: #11966

@pailhead
Copy link
Contributor

pailhead commented Feb 22, 2018

Actually, the reason for removing vertices is the data-structure of the geometry is irrelevant. What is relevant is the number of primitives rendered.

Any chance you could elaborate a bit?
I’ve tried my best to ask a meaningful question:

https://stackoverflow.com/questions/48798175/how-are-vertices-transformed-in-webgl-in-indexed-and-non-indexed-geometries

My understanding of this is that your statement is wrong, but it is complicated and I’m not sure I understand it right.

@WestLangley
Copy link
Collaborator

@pailhead The proposal is to tally only triangles when rendering Meshes, tally only line segments when rendering Lines, and tally points when rendering Points.

If you think doing so is incorrect or misleading, you can propose an alternate PR if you want.

@pailhead
Copy link
Contributor

@WestLangley

I am trying to learn the reasoning behind something like this, and to better understand how the underlying mechanics work (WebGL, GPUs). I did open an alternate PR that just got closed in favor of this, so I don't find your suggestion constructive at all :(

Where should I ask my question? Your answer reads as "not here", "i don't have time to explain myself" etc.

I had one (limited) understanding of how webgl and gpus work. I looked at the info numbers pre this PR and got confused. I asked first on Slack, then on Discourse, then here in the mentioned issue, then once again in my:

...you can propose an alternate PR if you want.

followed by the stack overflow question, and finally here in this PR. From the viewpoint of my question this PR is alternate, not the other one. The other PR wasn't even meant to be accepted but start this conversation, and hopefully answer some questions.

Questions like:

Actually, the reason for removing vertices is the data-structure of the geometry is irrelevant. What is relevant is the number of primitives rendered.

Why is it irrelevant, why is the other relevant?

It was important enough for you to outline this here, but asking to go further and elaborate is a dead end. My understanding of the SO answer is that it is not irrelevant, that cache plays a great role here, and that there's been research done on how to best use this cache. For example and convenience this:
https://tomforsyth1000.github.io/papers/fast_vert_cache_opt.html

Because these were linked, and because they seem to contradict your comment I think they make the reasoning behind this PR more confusing.

Without links to SO, the papers, and your comment one may think.

Oh, the vertices numbers were wrong, i see they fixed that by removing them, probably weren't relevant

With the links

Oh, the vertices are removed. They seem to be relevant, but why - seems overly complicated, and i probably dont care about it.

With your comment

Oh, the vertices got removed, good, they were never relevant to begin with.

Everything combined:

😕

@pailhead
Copy link
Contributor

pailhead commented Feb 22, 2018

tl:dr;

  • By all means, land this PR as is, yesterday!
  • Leave some breadcrumbs for the future graphics enthusiast and student trying to learn WebGL. I think there are lots of people learning graphics with three.js. Having something "just work" without explanation doesn't really help people learn.

@WestLangley

If you think doing so is incorrect or misleading, you can propose an alternate PR if you want.

I don't want to do a bunch of code for someone to come and say "Its irrelevant". The only misleading thing here I think was your comment and no explanation. Where is the proper place to address it? Writing a bunch of code just to reference some papers seems unproductive. I guess i could have quoted you, bolded the irrelevant part and just said may be wrong ^ consult foo,bar and baz but dunno, that seems a bit confrontational.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2018

@pailhead I think i understand your motivation behind your posts but i don't think that PR's should contain a tutorial for the interested reader. If users want to learn fundamental concepts of WebGL, they should study the relevant literature, documentation, specs and papers about computer graphics in general, and WebGL in particular. Reading issues and PRs is a useful addition but it can't replace the mentioned stuff.

@WestLangley
Copy link
Collaborator

@pailhead I did not close your PR. I also do not know the answers to many of your questions. When I do, I do my best to answer them. Intonation does not translate well in text. My intention is to be polite. Please give me the benefit of the doubt.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 22, 2018

I did not close your PR.

Yep, i was it 😇 . Because i think this one does exactly the same: Removing info.render.vertices. We don't need two PRs for this.

@pailhead
Copy link
Contributor

pailhead commented Feb 22, 2018

Thank you for reiterating the tone :)

@Mugen87
I wasn't thinking of a tutorial, but a few notes / thoughts wouldn't hurt.

Since the renderer is the only thing that instantiates geometries, maybe it would be useful to do a one time count of how many vertices are in memory or something. Not relevant to the render calls that the renderer does (drawing) but relevant to what it does automagically by creating the geometries on the gpu. Anyhow, this probably makes the level of breadcrumbs i can live with :)

And thanks for trying to understand where i come from, i think i'm trying to improve a process, or at least understand it better.

I did not close your PR.

Just to clarify, i wasn't referring to the other PR but a fictional PR which would be made just to ask a question. I was trying to address the way of asking a question, apologies for the confusion :)

@mrdoob mrdoob added this to the r91 milestone Feb 24, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 25, 2018

Very nice! 😊

@mrdoob mrdoob merged commit d97f368 into mrdoob:dev Feb 25, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 25, 2018

Thanks!

@WestLangley
Copy link
Collaborator

As suggested in #13316 (comment), I now definitely think triangles is a better term than faces. We are counting primitives.

And you could replace the word segments with lines, too -- even though it can be slightly confusing.

triangles, lines, and points. That's what I would vote for.

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.

5 participants