Modules/rune #80

houruomu · 2021-07-28T09:06:45Z

Description

The rune library has been hard-coded into the frontend of Source Academy. This branch migrated the rune library to the 'rune' module that is fully compliance with the module specifications.

A refactoring and redesign of the code is done, and the current code follows KISS and DIY. It should make future development and maintenance easier.

Fixes # (issue)

Implementation of the 'rune' module

Notice that the z-axis interpretation is different from the previous library. Previously an orthogonal camera was used, and currently a perspective camera is used.

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Test A

import { beside, stack, red, heart, pentagram, rcross, blue, ribbon, overlay_frac, anaglyph } from 'rune';
const r = beside(stack(red(heart), pentagram),stack(rcross,blue(ribbon)));
let s = overlay_frac(0.5, r, r);
s = overlay_frac(0.666, s, r);
anaglyph(s);

Test B

import { beside, stack, red, heart, pentagram, rcross, blue, ribbon, overlay_frac, anaglyph, hollusion } from 'rune';
const r = beside(stack(red(heart), pentagram),stack(rcross,blue(ribbon)));
let s = overlay_frac(0.5, r, r);
s = overlay_frac(0.666, s, r);
hollusion(s);

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

solarrabbit99

Nice job! I just have the following few queries, some of which requires prof's input, marked with (Require Prof's advice):

Major:

(Require Prof's advice) show(rune) seems obsolete. It does nothing from what I see from functions.ts, and that tab is spawned and the rune is rendered as long as the last statement returns a rune.
However, it shouldn't be useless as assignment of variable does not return any value and hence, may be required when displaying runes that are assigned to a variable. For example, const a = circle; show(a);.
I'm suggesting that it should be required for show to be the last statement for rune to be displayed.
Also, by making show(rune) do nothing, show(show(...(show(rune))) displays rune normally, where it shouldn't.
stack_frac(frac, rune1, rune2) is allowing frac argument to be <0 or >1, where logically and in documentation shouldn't. Same for beside_frac.
overlay and overlay_frac not working as expected. Example: overlay(circle, circle)
Expected:

Actual:

Minor:

gl.createShader(type) seems to return a null value at times after prolong use of the playground. A developer stage error page will pop out, but I'm not sure if the problem will persist in production and crash SA. Since the functions are from the curve module and that there are yet to be reports on this for curve, I assume it should not persist.
(Require Prof's advice) hollusion seems to just move the rune left and right, is that the intended behaviour? I put it under minor as it seems to be so in the old js-slang library too, but the function doesn't seem to add any educational value.
(Require Prof's advice) I briefly remember there is a function to view 3D runes' overlays from the side. Not sure if that function is only available in one of the missions/quests. It may be useful for testing.

martin-henz · 2021-07-30T09:03:58Z

Regarding minor question 2: hollusion: This is for 3D runes, created by overlay and overlay_frac. The "move rune left and right" should have a 3D effect if you view the rune using hollusion. Can you verify that that's the case?

martin-henz · 2021-07-30T09:14:54Z

Regarding minor question 3: 3D runes: No, the only way to see 3D runes is with hollusion (with the left-to-right shifting with 3D effect) and the show function. Note that "show" will use grey-scale.

To answer your question on "view from the side": You memory is wrong: The instructions to the mission show a side-view for illustration, but the Source Academy does not support that.

I forgot: We also have the function anaglyph, which uses cyan-magenta and is to be used with 3D-glasses.

See here for the documentation: https://docs.sourceacademy.org/RUNES/index.html

The reviewer needs to make sure that the full specs are implemented in the module.

martin-henz · 2021-07-30T09:24:57Z

Regarding major question 1: In the following I'm referring to the documentation when I talk about "rune" and "picture": https://docs.sourceacademy.org/RUNES/global.html#show

I'm ok with conflating the notions of "rune" and "picture" for the case of "show". That's consistent with the current specs. In that case, "show" is the identity function, and "hollusion" and "anaglyph" produce "more fancy versions" of the rune.

solarrabbit99 · 2021-07-30T10:49:10Z

To answer your question on "view from the side": You memory is wrong: The instructions to the mission show a side-view for illustration, but the Source Academy does not support that.

I forgot: We also have the function anaglyph, which uses cyan-magenta and is to be used with 3D-glasses.

See here for the documentation: https://docs.sourceacademy.org/RUNES/index.html

The reviewer needs to make sure that the full specs are implemented in the module.

Ahh ok, I asked the question as I couldn't find the function in the documentation. I did do a full spec review on everything that is implemented.

Regarding minor question 2: hollusion: This is for 3D runes, created by overlay and overlay_frac. The "move rune left and right" should have a 3D effect if you view the rune using hollusion. Can you verify that that's the case?

I have tested it on the current js-slang library and I get what you mean by 3D effect.
Apparently a second argument (likely to affect the runes' moving speed) is required for the current js-slang runes library but it is not specified so in the documentation. This PR's implementation is closer to what is documented and only accepts 1 argument.

// The current PR's implementation
hollusion(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle)))));
// The current js-slang implementation
show(hollusion(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle)))), 2));

However, this PR has yet to implement overlay correctly so I can't test if hollusion is implemented correctly too.

Regarding major question 1: In the following I'm referring to the documentation when I talk about "rune" and "picture": https://docs.sourceacademy.org/RUNES/global.html#show

I'm ok with conflating the notions of "rune" and "picture" for the case of "show". That's consistent with the current specs. In that case, "show" is the identity function, and "hollusion" and "anaglyph" produce "more fancy versions" of the rune.

I am currently against this. Blurring the distinction between the two might have nonsensical implications on visualiser functions and is possibly much more challenging to implement. If a hollusion and anaglyph is treated as a rune, as it is currently in the PR, then the following will be allowed:

beside(hollusion(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle))))), circle);
beside(anaglyph(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle))))), circle);

However, the current implementation of beside returns a rune (which is by current definition, non-animated), which will break the properties of overlay and hollusion and anaglyph functions. Both of the above functions returns the following:

martin-henz · 2021-07-30T12:56:48Z

To answer your question on "view from the side": You memory is wrong: The instructions to the mission show a side-view for illustration, but the Source Academy does not support that.
I forgot: We also have the function anaglyph, which uses cyan-magenta and is to be used with 3D-glasses.
See here for the documentation: https://docs.sourceacademy.org/RUNES/index.html
The reviewer needs to make sure that the full specs are implemented in the module.

Ahh ok, I asked the question as I couldn't find the function in the documentation. I did do a full spec review on everything that is implemented.

Regarding minor question 2: hollusion: This is for 3D runes, created by overlay and overlay_frac. The "move rune left and right" should have a 3D effect if you view the rune using hollusion. Can you verify that that's the case?

I have tested it on the current js-slang library and I get what you mean by 3D effect.
Apparently a second argument (likely to affect the runes' moving speed) is required for the current js-slang runes library but it is not specified so in the documentation. This PR's implementation is closer to what is documented and only accepts 1 argument.
// The current PR's implementation
hollusion(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle)))));
// The current js-slang implementation
show(hollusion(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle)))), 2));
However, this PR has yet to implement overlay correctly so I can't test if hollusion is implemented correctly too.

Regarding major question 1: In the following I'm referring to the documentation when I talk about "rune" and "picture": https://docs.sourceacademy.org/RUNES/global.html#show
I'm ok with conflating the notions of "rune" and "picture" for the case of "show". That's consistent with the current specs. In that case, "show" is the identity function, and "hollusion" and "anaglyph" produce "more fancy versions" of the rune.

I am currently against this. Blurring the distinction between the two might have nonsensical implications on visualiser functions and is possibly much more challenging to implement. If a hollusion and anaglyph is treated as a rune, as it is currently in the PR, then the following will be allowed:
beside(hollusion(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle))))), circle);
beside(anaglyph(scale(0.1, overlay(blue(circle), overlay(circle, orange(circle))))), circle);
However, the current implementation of beside returns a rune (which is by current definition, non-animated), which will break the properties of overlay and hollusion and anaglyph functions. Both of the above functions returns the following:

Yes, makes sense. Let's not conflate the notions of "picture" and "rune". Let's stick to the current specs. @houruomu can you take another look?

houruomu · 2021-07-31T00:21:07Z

About show() returns a rune: I will make changes accordingly. At this moment, I will perhaps hack to fix this by adding another field in the rune class to differentiate the rune produced by show(), anaglyph(), hollusion() and those "pure" runes. Once the improvement in issue 69 (i.e. the shared state) is implemented, the implementation of the showing functions will be reworked entirely.

About "unexpected behavior" of overlay() (request for comment): The rune library behaves differently from the js-slang implementation when it comes to interpreting the depth field, and what you saw is expected. In the rune library, the camera is perspective while in the js-slang implementation the camera is isometric. In the rune library, the camera has a 90-degree FoV and is placed at (0,0,-1) facing the positive z-axis. Additionally, in the rune library, the camera does not use a gray gradient to differentiate depth. Hence, when you overlay two circles, the top black circle is placed at the plane z = -0.5 and will cover the whole camera view. I think the documentation did not specify how depth should be interpreted and hence both versions are correct (correct me if I remember this wrongly). To verify overlay() does what I said, please try hollusion(overlay(ribbon, ribbon)), which will show this:

About beside_frac and stack_frac accept fractions outside of (0,1): I will add this constraint in.

houruomu · 2021-07-31T05:22:19Z

Major (1,2) fixed. Major (3) requires comment.

Excited to introduce a new feature to use images to create runes:

import { stack,ribbon, show,beside, yellow, get_image_rune, heart } from 'rune';

const r = get_image_rune("https://i.imgur.com/t4FshLk.jpeg");
show(beside(stack(yellow(heart), ribbon),stack(r,ribbon)));

MarcusTXK · 2021-07-31T06:23:09Z

Major (1,2) fixed. Major (3) requires comment.

Excited to introduce a new feature to use images to create runes:
import { stack,ribbon, show,beside, yellow, get_image_rune, heart } from 'rune';

const r = get_image_rune("https://i.imgur.com/t4FshLk.jpeg");
show(beside(stack(yellow(heart), ribbon),stack(r,ribbon)));

Hi houruomu, great work doing up this library (and so quickly too). I think zhenghanlee has tested and commented quite thoroughly, couldn't really find any other problems at the moment (will take some more time to test to see if I can find more bugs).

For the most recent feature you added, when running, I get a blue screen as seen in this gif:

The way to replicate this is to hard reload the browser, before running the code.

As seen in the console.log, and is likely related to the WebGL: INVALID_VALUE: texImage2D: invalid image warning.
My guess is this is likely caused by the image not having been loaded in time.

houruomu · 2021-08-02T09:12:39Z

Please help review, thanks. Major 1-3 fixed. Now should have feature parity.

Known issue to be postponed in future PR:

Let get_image_rune show some default loading texture before image fully loaded.

Features postponed to future PR:

perspective/render from other camera angles
do not use return result for web rendering, instead use shared state.

solarrabbit99

Nice! All works as expected. I just have 2 comments but they rather minor, nothing breaking.

Minor:

anaglyph(show(scale(0.5, overlay(scale(0.25, orange(circle)), circle)))); shows error message "Error: show expects a rune as argument." instead of "Error: anaglyph expects a rune as argument."
hollusion is now much more subtle than it used to be. I personally prefer it to be drastic. What do prof think?

hollusion(overlay(scale(0.7, ribbon),
    overlay(scale(0.8, blue(ribbon)), 
    overlay(scale(0.95, orange(ribbon)), ribbon))), 1);

Before:

After:

martin-henz · 2021-08-02T23:03:11Z

Great. I'm merging and are making issues out of the comments by @ZhengHanLee

TobiasWrigstad · 2021-08-13T10:32:20Z

@houruomu This is super nice! I was bitten by get_image_rune being renamed for a little while.

Question: in Chapter 2.2.4 of SICP, we apply some transformations like squash_inwards to runes. Are these functions supported anywhere in the current system? I am looking to recreate figures 2.9-2.14 from SICP. Thanks!

martin-henz · 2021-08-13T11:27:08Z

We havent ported the painter stuff to Source Academy yet. That may be something for Ruomu in the next couple of months if he has the time. But probably not in time for our book. I think we'll end up drawing these "by hand" as we've done in the past.

TobiasWrigstad · 2021-08-13T11:33:27Z

OK, got it. (That was what I expected!) När du har kontakt med oss på Uppsala universitet med e-post så innebär det att vi behandlar dina personuppgifter. För att läsa mer om hur vi gör det kan du läsa här: http://www.uu.se/om-uu/dataskydd-personuppgifter/ E-mailing Uppsala University means that we will process your personal data. For more information on how this is performed, please read here: http://www.uu.se/en/about-uu/data-protection-policy

houruomu added 12 commits July 27, 2021 13:23

initialize the rune module

dd58a3a

update reference

f3c000e

add development journal

3e81065

color functions implemented

4777777

update the journal

4338726

refactor the shader initialization code

d6df796

create anaglyph

39318f8

implement hollusion

8cf819f

fix various bugs

7628e7f

update journal

cba8f0f

touch up documentation

d618bb2

make circle nicer

a493978

solarrabbit99 self-assigned this Jul 28, 2021

solarrabbit99 self-requested a review July 28, 2021 11:27

martin-henz requested a review from MarcusTXK July 29, 2021 01:27

solarrabbit99 reviewed Jul 30, 2021

View reviewed changes

martin-henz marked this pull request as draft July 30, 2021 12:57

houruomu added 3 commits July 31, 2021 11:43

now the return value of show,anaglyph,hollusion cannot be used as runes

4f46fd1

limit the input frac range

b5250b1

implement the image rune feature

96efc0f

samuelfangjw mentioned this pull request Aug 2, 2021

WEB: Migrate snippets with runes to new runes modules source-academy/sicp#587

Closed

fix anaglyph for image runes

8c8c1b0

samuelfangjw mentioned this pull request Aug 2, 2021

Add import statement for runes when required source-academy/sicp#588

Merged

change camera to isometric

0cbb959

martin-henz marked this pull request as ready for review August 2, 2021 10:32

martin-henz requested a review from solarrabbit99 August 2, 2021 10:33

solarrabbit99 approved these changes Aug 2, 2021

View reviewed changes

martin-henz merged commit dae533c into source-academy:master Aug 2, 2021

This was referenced Aug 2, 2021

[RUNE]: small bug in error message #82

Closed

[RUNE]: hollusion more drastic? #83

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modules/rune #80

Modules/rune #80

houruomu commented Jul 28, 2021 •

edited

Loading

solarrabbit99 left a comment •

edited

Loading

martin-henz commented Jul 30, 2021 •

edited

Loading

martin-henz commented Jul 30, 2021 •

edited

Loading

martin-henz commented Jul 30, 2021

solarrabbit99 commented Jul 30, 2021 •

edited

Loading

martin-henz commented Jul 30, 2021

houruomu commented Jul 31, 2021

houruomu commented Jul 31, 2021

MarcusTXK commented Jul 31, 2021 •

edited

Loading

houruomu commented Aug 2, 2021 •

edited

Loading

solarrabbit99 left a comment

martin-henz commented Aug 2, 2021

TobiasWrigstad commented Aug 13, 2021

martin-henz commented Aug 13, 2021

TobiasWrigstad commented Aug 13, 2021 via email

Modules/rune #80

Modules/rune #80

Conversation

houruomu commented Jul 28, 2021 • edited Loading

Description

Implementation of the 'rune' module

How Has This Been Tested?

Checklist:

solarrabbit99 left a comment • edited Loading

Choose a reason for hiding this comment

martin-henz commented Jul 30, 2021 • edited Loading

martin-henz commented Jul 30, 2021 • edited Loading

martin-henz commented Jul 30, 2021

solarrabbit99 commented Jul 30, 2021 • edited Loading

martin-henz commented Jul 30, 2021

houruomu commented Jul 31, 2021

houruomu commented Jul 31, 2021

MarcusTXK commented Jul 31, 2021 • edited Loading

houruomu commented Aug 2, 2021 • edited Loading

solarrabbit99 left a comment

Choose a reason for hiding this comment

martin-henz commented Aug 2, 2021

TobiasWrigstad commented Aug 13, 2021

martin-henz commented Aug 13, 2021

TobiasWrigstad commented Aug 13, 2021 via email

houruomu commented Jul 28, 2021 •

edited

Loading

solarrabbit99 left a comment •

edited

Loading

martin-henz commented Jul 30, 2021 •

edited

Loading

martin-henz commented Jul 30, 2021 •

edited

Loading

solarrabbit99 commented Jul 30, 2021 •

edited

Loading

MarcusTXK commented Jul 31, 2021 •

edited

Loading

houruomu commented Aug 2, 2021 •

edited

Loading