Improve demo, simplify user experience #20

rylandg · 2018-12-15T23:45:26Z

I know this is a big PR and I apologize for that. These are changes that accumulated during kubecon and while I was flying. It really adds to the demo and enables some even better changes down the road. I'm going to list the changes at a high level here.

It's now possible to run against a local express server. Deploying with all of these dependencies became very cumbersome so it's very nice to have.
I've split up the repo into logical directories. This allows us to shard our npm dependencies which SUBSTANTIALLY reduced deploy times. You can now redeploy the backend function in 5-6 seconds and the frontend function in ~15 seconds.
I've also added a number of npm scripts that saves time when a new user is running the demo.
I've added custom textures which are served from the servePage endpoint. This improves the visual appeal of the demo and is the first step to making it look like real terrain.
Because sandbox has 1 concurrency by default, simply fetching resources from the HTML was causing 429 errors. I now force assets to load serially and added a sleep when scaling Workers.
I've completely removed the need for a user to supply custom endpoints of any type. As long as they've used bn login the demo will work
Terrain is now chunked vertically in addition to horizontally. This has a negative effect on client performance currently, but there are ways we can reduce that overhead. Regardless, it's worth it for the much more appealing terrain.

Although I thought it would be too difficult to split the patches I put a lot of effort into isolating the commits. The changes are far less overwhelming when you look at a commit by commit basis.

This PR also needs binaris/binaris#418 to function

vogre

I didn't understand why there was need for binaris/binaris#418 (and subsequent binaris/binaris#419), what CORS broke?

vogre · 2018-12-16T09:10:38Z

fractal-terrain-generation/builder.js

+}
+
+async function getPublicPath(accountID, endpoint) {
+  process.env.BINARIS_INVOKE_ENDPOINT = endpoint;


Is overriding process.env.BINARIS_INVOKE_ENDPOINT before storing it in savedEndpoint a bug?

Also, this assumes that binaris/sdk/url behaves in a certain way - if you need a specific functionality from the SDK maybe this function should be in the SDK?

It's not a bug. Our SDK relies on Environment variables and cannot be overridden with passed values. I definitely agree this could prompt a change but that could entail a lengthy discussion. How about we do that in parallel?

It does assume but the alternative is exposing bloat to the user. For now I'm ok with this hack until we fix the underlying issues

Do we still need this whole function with the sandbox shutdown?

rylandg

@vogre it broke because instead of the user supplying the endpoint for the function it's automatically generated using our internal SDK logic. As of now our SDK does not use the same endpoint that we print to the user. Therefore them pasting it in the browser forces a redirect and therefore CORS.

rylandg · 2018-12-17T02:41:21Z

fractal-terrain-generation/builder.js

+}
+
+async function getPublicPath(accountID, endpoint) {
+  process.env.BINARIS_INVOKE_ENDPOINT = endpoint;


It's not a bug. Our SDK relies on Environment variables and cannot be overridden with passed values. I definitely agree this could prompt a change but that could entail a lengthy discussion. How about we do that in parallel?

It does assume but the alternative is exposing bloat to the user. For now I'm ok with this hack until we fix the underlying issues

vogre · 2018-12-17T06:40:53Z

it looks like a bug. If I do

a = newValue;
oldValue = a;
a = oldValue;

then the last two lines have no effect.

The SDK uses environment variables in our environment: https://github.com/binaris/realms/pull/68/files - when running in a Binaris function it has the correct values.

From your use-case it looks like you are trying to use the SDK for building your code to run in multiple realms (sandbox, prod) and the assumption during binaris/binaris#392 was that SDK is for use in Binaris functions and not for build systems -- if this is wrong we need to fix the SDK - the current suggested change will break if the SDK internals change in any way (and this means we are not allowed to change them?).

As a quick fix that does not entail a lengthy discussion, which may also be practical for users: how about we serve the HTML templated from the function instead of doing replace-in-file during the build stage? Most web frameworks support templating, and our examples can also demonstrate it.

rylandg · 2018-12-17T17:48:35Z

I was 100% wrong, that is a bug and I'll fix it. Sorry for not realizing it immediately.
(Regarding SDK) Indeed I'm using for build. I agree it's crappy but the alternative puts more burden on the user. I'm trying to minimize the overhead of using/running this demo as much as possible. I 100% believe we need to not only fix but separate out the SDK.
I'm not sure I 100% follow. I want to avoid more complex templating solutions because I'm doing a very basic sed and adding more bloat and abstraction will increase the barrier of entry for users. I think you're suggesting that we deploy with the HTML template and then at invocation time inject the accountID into the HTML before serving. Is accountID available in runtime?

vogre · 2018-12-17T19:22:05Z

Is accountID available in runtime?

Good question - this was discussed but somehow missed - I've opened PRs for spec and implementation.

I want to avoid more complex templating solutions because I'm doing a very basic sed

In my opinion sed is confusing and I wouldn't recommend users to build their examples with a build process that requires replacing text in files, and templating is familiar if you used Jinja2 or ERB or any other templating solution.

It would add about 10 lines of code to our demo web-server servePage.js, but will be useful for examples.

rylandg · 2018-12-17T20:08:26Z

Once we have accountID available in the function I will happily switch the implementation.

Alternatively I can add BINARIS_ACCOUNT_ID as a env var in binaris.yml and then programmatically inject it during backend function deployment.

Thoughts?

vogre · 2018-12-17T21:16:00Z

I think it's preferrable to wait for account ID in the function to avoid changing the example twice.

rylandg · 2018-12-17T21:24:09Z

I have no issue with that but it means the PR might grow more. I've been spending my free time improving this example.

vogre · 2018-12-19T18:58:46Z

BINARIS_ACCOUNT_ID is now available in environment

rylandg · 2018-12-19T19:31:54Z

@vogre Awesome, will update.

rylandg · 2019-01-02T23:24:01Z

@vogre I made the improvements we discussed and you helped me with. We now use dot for simple templating. I'm also considering splitting the servePage function into a completely different "app" and then depending on it from this one.

vogre · 2019-01-03T16:32:22Z

@rylandg the last change looks good, do you want me to review the whole PR now?

rylandg · 2019-01-03T18:11:14Z

@vogre that would be great

vogre

Thanks!

I've left a few comments, mostly requests to revisit since this was done for sandbox and now in public beta perhaps not needed.

I'd really prefer the getPublicPath function to not do the environment variables manipulation so that users don't copy it, is was done in the first place because of sandbox IIUC, the only major request is to update it before merge.

vogre · 2019-02-10T16:48:17Z

fractal-terrain-generation/fractal_backend/fractal.js

+  const genStartTime = process.hrtime();
+
+  const { blockCount, data, maxHeight } = noiseGen(
+    parseInt(xPos, 10), parseInt(yPos, 10),


I think it would be cleaner to have all string->int conversion in the beginning of the handler, (even if some arguments are now passed as strings (numTex, downscale) they work due to conversion, but it's not optimal).

vogre · 2019-02-10T16:51:16Z

fractal-terrain-generation/fractal_backend/noiseGen.js

-  if (maxHeight % size !== 0) {
-    maxHeight = Math.ceil(maxHeight / size) * size;
+
+  if ((minHeight) > (cY + size)) {


Nit: extra parentheses?

vogre · 2019-02-10T16:56:07Z

fractal-terrain-generation/src/main.js

-const game = setupDemo();
-
-game.animate();
+Promise.resolve(setupDemo()).then((game) => {


Can be setupDemo().then((game) => { ?

vogre · 2019-02-10T16:56:41Z

fractal-terrain-generation/src/shaders/internal_compute_shader.frag

+}
+
+void main() {
+	vec4 lights = vec4(0.0, 0.0, 0.0, 1.0);


Nit: mixed tabs and spaces.

vogre · 2019-02-10T16:57:08Z

fractal-terrain-generation/src/shaders/internal_compute_shader.vert

+
+void main() {
+    vUv = uv;
+		vNormal = (modelMatrix * vec4(normal, 0.0)).xyz;


Nit: mixed tabs and spaces.

vogre · 2019-02-10T17:10:39Z

fractal-terrain-generation/fractal_backend/fractal.js

+    headers = makeHeaders(blockCount, maxHeight, respBody.length, genDataTimeStr);
+    statusCode = 200;
+  }
+  if (ctx.request.query.express_server) {


What do you think about instead of adding a fake express_server, pass a HTTPResponse constructor and move this to express?

vogre · 2019-02-10T17:12:31Z

fractal-terrain-generation/fractal_backend/fractal.js

+    const genStr = (failedGenTime[0] * 1000) + (failedGenTime[1] / 1000000);
+    headers = makeHeaders(blockCount, maxHeight, 0, genStr);
+    respBody = new Buffer(2);
+    statusCode = 200;


Code-style: my preference would be to have an early-return here (but would require moving into a separate function).

vogre · 2019-02-10T17:18:36Z

fractal-terrain-generation/dist/servePage.js

@@ -15,14 +17,21 @@ exports.handler = async (body, ctx) => {
    resourcePath = '/index.html';
  }

-  const webpage = await fs.readFile(`${prefix}${resourcePath}`);
+  let webPage;


Since this code is mostly shared with react-serverless, perhaps also rename to webResource.

Also if this is to be a recommended code for using in a Binaris function perhaps makes senses moving it into a unified place.

vogre · 2019-02-10T17:28:22Z

fractal-terrain-generation/builder.js

+}
+
+async function getPublicPath(accountID, endpoint) {
+  process.env.BINARIS_INVOKE_ENDPOINT = endpoint;


Do we still need this whole function with the sandbox shutdown?

vogre · 2019-02-10T17:30:37Z

fractal-terrain-generation/builder.js

+  }
+  const accountID = await getAccountId(undefined);
+  const FRACTAL_ENDPOINT = await getFractalURL(accountID);
+  const PUBLIC_PATH = (await getPublicPath(accountID, ' ')).slice(8);


Why do we slice(8) the https:// away for webpack? Can we get it through a URL object instead?

michaeladda · 2019-02-25T12:13:29Z

@rylandg any update on this?

rylandg · 2019-02-25T17:23:50Z

@michaeladda

Yes. It's a complex set of fixes and I've just been prioritizing other things. I viewed this as overall lower importance considering that the example already works and these are just further features/improvements.

rylandg added 8 commits December 14, 2018 14:13

Add local server support

651e3e9

support vertical chunking

fdc7f03

make deploying modular and fast, new npm scripts

6d75f79

load textures from our endpoint

0b50a51

rework asset loading to avoid concurrency issues

1c7a155

try and simplify user experience

80e728b

update README

f2a2a47

force realm in builder

2ef49bd

rylandg mentioned this pull request Dec 15, 2018

Update default endpoints binaris/binaris#418

Closed

rylandg added 3 commits December 15, 2018 15:56

fix mistake in package.json

942adca

Fix materials on PC

51ea0ce

bump up time again to avoid 429

2fc4d0b

vogre reviewed Dec 16, 2018

View reviewed changes

rylandg commented Dec 17, 2018

View reviewed changes

fix env var bug in builder

f0bad0b

use standard version of CLI

b02e3f1

improvements based on review

41feeb4

vogre suggested changes Feb 10, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve demo, simplify user experience #20

Improve demo, simplify user experience #20

rylandg commented Dec 15, 2018 •

edited

Loading

vogre left a comment

vogre Dec 16, 2018

rylandg Dec 17, 2018

vogre Feb 10, 2019

rylandg left a comment

rylandg Dec 17, 2018

vogre commented Dec 17, 2018

rylandg commented Dec 17, 2018

vogre commented Dec 17, 2018

rylandg commented Dec 17, 2018

vogre commented Dec 17, 2018

rylandg commented Dec 17, 2018

vogre commented Dec 19, 2018

rylandg commented Dec 19, 2018

rylandg commented Jan 2, 2019

vogre commented Jan 3, 2019

rylandg commented Jan 3, 2019

vogre left a comment

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

vogre Feb 10, 2019

michaeladda commented Feb 25, 2019

rylandg commented Feb 25, 2019

Improve demo, simplify user experience #20

Are you sure you want to change the base?

Improve demo, simplify user experience #20

Conversation

rylandg commented Dec 15, 2018 • edited Loading

vogre 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

rylandg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogre commented Dec 17, 2018

rylandg commented Dec 17, 2018

vogre commented Dec 17, 2018

rylandg commented Dec 17, 2018

vogre commented Dec 17, 2018

rylandg commented Dec 17, 2018

vogre commented Dec 19, 2018

rylandg commented Dec 19, 2018

rylandg commented Jan 2, 2019

vogre commented Jan 3, 2019

rylandg commented Jan 3, 2019

vogre 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

michaeladda commented Feb 25, 2019

rylandg commented Feb 25, 2019

rylandg commented Dec 15, 2018 •

edited

Loading