Better shader docs #3154

machenmusik · 2017-10-11T02:24:27Z

donmccurdy · 2017-10-11T04:31:24Z

docs/components/material.md

+  },
+
+  init: function () {
+    this.material = this.el.getOrCreateObject3D('mesh').material = new THREE.ShaderMaterial({


This predates your PR, but i think getOrCreateObject3D('mesh').material is a bad pattern. It screws with components waiting for object3dset, for example. IMO this initializer should see if a mesh is available already, and if not wait for the object3dset method.

(this has the added benefit of making it compatible with custom models, if you use a traverse(), which registerShader cannot do)

How does it mess with components waiting for object3dset?

Point is, the framework should abstract all this away so you can say shader:displacement and let A-frame worry about when. That is what registerShader allows, and this entire approach completely fails to do.

if you use a traverse(), which registerShader cannot do

Shouldn't the material component be the one doing that?

My thought was a simple recursive:true (or we can still call it traverse). I am guessing we can detect the aframe entity object3D structure and refuse to cascade, but I haven't looked.

You can always make your own replacement for the material component, which obviously you do by default, but for the simple example cases that seems like overkill.

What I am not sure about is whether the model object trees have consistent DOM-ish naming, for better declarative mapping etc. I think I saw discussion on that in another issue/PR

I've found that when I need a custom material, I just use a raw component. I think @donmccurdy does the same? It's simpler than hooking into material. Should we recommend that?

The idea is that if you only need to use your own (vertex and fragment) shader, that is most easily done with registerShader and then material="shader:my-shader". The examples that were given previously are actually easier to do that way (which is hopefully illustrated).

Other examples where you only need shader and not custom material, besides the ones listed, include the typical top/bottom and left/right stereo shaders we used to recommend separate geometry and/or components for.

If registerShader doesn't work for various reasons, then it does recommend creating a custom component (with this example).

I did ask for an example that actually requires custom component, but so far no one has suggested one.

Nevermind, I saw this is already done as a raw component. I thought it was trying registerShader.

machenmusik · 2017-10-11T06:15:02Z

OK so -

We seem to agree these docs are an improvement.

Both the examples (grid and displacement) are done both as registered shader and custom component, so we actually have @donmccurdy's comment whynotboth already.

Ok to merge while working #3155?

donmccurdy · 2017-10-11T15:05:21Z

I haven't had the chance to do a thorough read-through yet, but assuming someone has reviewed for any errors then +1 from me for merging and not waiting on the other discussions.

dmarcos · 2017-10-12T02:31:31Z

docs/components/material.md


 varying vec2 vUv;
 uniform vec3 color;
-uniform float time;


time1000 is a weird name. Let’s keep time. It is already consistent with other time related variables that use miliseconds in the code base

So the reason I didn't use time is because that is commonly used to represent time, but in seconds not milliseconds. Unfortunately the time type is processed as milliseconds, so I wanted to pick a different name and convert to the time in seconds that typical sharers expect. Maybe timeMsec or something more descriptive?

To clarify, there is inherent support for type time, without using a tick() handler to update anything, but it is in milliseconds rather than seconds (which is what you generally see in common sharers from Shadertoy, Shaderfrog, etc.)

In the DOM and A-Frame we use miliseconds for time. I would keep the name time

I may not be doing a good job of explaining.

The shader examples that are time-varying already use a uniform named time that is in seconds. Rather than trying to force already working shaders to rename, what I am illustrating is how to do a simple two line addition that allows the shader code to be reused without modification.

Changed to timeMsec, and added comments in the shader to explain the conversion to seconds.

donmccurdy · 2017-10-12T04:15:57Z

docs/components/material.md


 ```js
-AFRAME.registerComponent('custom-material', {
+<script src="https://rawgit.com/aframevr/aframe/master/dist/aframe-master.min.js"></script>


Any reason we need to be suggesting the master version in the docs?

@ngokevin do versions get automatically find/replaced when we do a release?

No reason other than not having to change it every release.

donmccurdy · 2017-10-12T04:18:17Z

docs/components/material.md

-  update: function () {
-    // Update `this.material`.
+Setting raw to true uses THREE.RawShaderMaterial instead of ShaderMaterial,
+so your shader strings are used as-is, for advanced shader usage.


I'd link to the docs for THREE.RawShaderMaterial and ShaderMaterial here.

so your shader strings are used as-is, for advanced shader usage.

maybe better to be explicit: "so built-in uniforms and attributes are not automatically added to your shader code."

donmccurdy · 2017-10-12T04:23:21Z

docs/components/material.md

+```glsl
+// vertex.glsl
+
+#pragma glslify: pnoise3 = require(glsl-noise/periodic/3d)


I probably wouldn't go to the trouble of explaining how to set it up here, but worth noting somewhere that:

This example uses Browserify and glslify to include glsl-noise as a dependency of our shader.

(similar options are available for rollup etc.)

dmarcos · 2017-10-12T05:26:53Z

We have an example of custom shaders already. Do we want to point to it in the docs? https://aframe.io/aframe/examples/test/shaders/

machenmusik · 2017-10-12T06:07:59Z

@dmarcos added that example

machenmusik · 2017-10-12T17:02:22Z

OK, I think this looks good now, unless someone has a real example to contribute of a custom shader that uses a type that registerShader doesn't support

ngokevin

Few changes. Thanks!

ngokevin · 2017-10-17T18:47:38Z

docs/introduction/shaders.md

@@ -0,0 +1,26 @@
+---


I think this part is too thin for a separate section. I think it's OK to be contained in the material section for now. Perhaps as the API and the ecosystem around shaders possibly matures, we move it out a separate section?

The non-duplicated part here that seems worth keeping is the reference to shader repositories like ShaderToy and/or ShaderFrog (there are probably others too) and pointers toward components that help with reusing those. Where would you suggest that appear in material documentation?

Perhaps when shaders are first introduced.

Looks like your shaderfrog gallery went away, was that intentional? Changed reference to just the shaderfrog site.

Yeah, it was an empty repo 🙄

ngokevin · 2017-10-17T18:50:34Z

docs/components/material.md


-* [Live demo](https://aframe-simple-shader.glitch.me/)
-* [Remix this on Glitch](https://glitch.com/edit/#!/aframe-simple-shader)
+* [Live demo](https://aframe-simpler-shader.glitch.me/)


All the Glitch demos can be listed in the examples array in the header:

examples: - title: Grid Glitch Shader url: https://glitch.com/edit/#!/aframe-simpler-shader?path=client/shader-grid-glitch.js:1:0

For ease of copy/pasting, I'd remove the GLSLify build step and use multiline strings.

It was that way originally (using GLSL-ify) so I left it that way; you can see that the new one I added as walkthrough does, in fact, use multi-line strings. Are you asking to make changes to the existing example instances that will retroactively alter documentation for prior versions?

You can remix the Glitch and not alter the old one.

FYI - current examples header looks pretty unattractive, and no other good examples to follow?

It's a data structure, it gets rendered underneath the table of contents as a list. I think there are two Glitches you listed, can add more as we go.

Just checked, there are more than two. Good list!

updated the Glitches I made, just the page and script, no more extraneous server and files

ngokevin · 2017-10-17T18:53:07Z

docs/components/material.md

+  },
+
+  init: function () {
+    this.material = this.el.getOrCreateObject3D('mesh').material = new THREE.ShaderMaterial({


I've found that when I need a custom material, I just use a raw component. I think @donmccurdy does the same? It's simpler than hooking into material. Should we recommend that?

ngokevin · 2017-10-17T20:36:05Z

Thanks, will work on merging this soon.

Could use more links!

donmccurdy reviewed Oct 11, 2017

View reviewed changes

donmccurdy mentioned this pull request Oct 11, 2017

Better documentation on AFRAME.registerShader() needed! #3151

Closed

donmccurdy mentioned this pull request Oct 11, 2017

Consider adding deferred and traversal / recursive application to material component. #3155

Closed

dmarcos reviewed Oct 12, 2017

View reviewed changes

donmccurdy reviewed Oct 12, 2017

View reviewed changes

ngokevin requested changes Oct 17, 2017

View reviewed changes

ngokevin added the needs changes label Oct 17, 2017

ngokevin approved these changes Oct 17, 2017

View reviewed changes

ngokevin force-pushed the better-shader-docs branch from 406ee93 to ee8bb35 Compare October 18, 2017 21:53

machenmusik added 13 commits October 18, 2017 14:56

Update material.md

c024413

Update material.md

2f67e5a

Update material.md

47e22ec

Update material.md

6a370ca

Update material.md

8d0c325

Update material.md

948b2e2

Create shaders.md

a387b47

Could use more links!

Update shaders.md

d6ac66a

Update shaders.md

d0c268d

Update shaders.md

b2459d1

Update shaders.md

c3fc4d2

Update material.md

a451ab4

Update material.md

072cda2

machenmusik and others added 6 commits October 18, 2017 14:56

Update material.md

50966cd

Update material.md

392f796

remove separate shaders section per discussion on PR

9653524

aframe-shadertoy-gallery is gone

948b3d1

add examples in header

16ae6e5

[docs] add don's displacement shader example to material docs

27c6ddb

ngokevin force-pushed the better-shader-docs branch from ee8bb35 to 27c6ddb Compare October 18, 2017 21:56

ngokevin merged commit 9f2f4d9 into aframevr:master Oct 18, 2017

Uh oh!

Better shader docs #3154

Better shader docs #3154

Uh oh!

Conversation

machenmusik commented Oct 11, 2017

donmccurdy Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machenmusik Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machenmusik commented Oct 11, 2017

donmccurdy commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos commented Oct 12, 2017

machenmusik commented Oct 12, 2017

machenmusik commented Oct 12, 2017

ngokevin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngokevin commented Oct 17, 2017

Labels

4 participants

donmccurdy Oct 11, 2017 •

edited

Loading

machenmusik Oct 11, 2017 •

edited

Loading