HTML Rendering System #48

notdanilo · 2019-11-18T15:09:20Z

Pull Request Description

This PR provides the implementation of the HTML Rendering System and its components:

HTMLRenderer: To render the camera transforms in the HTMLScene div and objects transforms in each HTMLObject div.
HTMLObject: Struct holding a HtmlElement and Transform
HTMLScene: A collection of HTMLObjects
Camera: Struct holding a Transform to position and orientate the camera, and a projection matrix.

Important Notes

To improve productivity, I've created a TestContainer when running wasm-pack test in a browser, so we can see its visual results.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the Rust, Scala, Java or Haskell style guides as appropriate.
All code has been tested where possible.

…er from XYZ to YXZ (like threejs)

codecov-io · 2019-11-18T15:34:23Z

Codecov Report

Merging #48 into master will increase coverage by 12.91%.
The diff coverage is 35.08%.

@@             Coverage Diff             @@
##           master      #48       +/-   ##
===========================================
+ Coverage    8.48%   21.39%   +12.91%     
===========================================
  Files           6       16       +10     
  Lines         283      575      +292     
  Branches       21       57       +36     
===========================================
+ Hits           24      123       +99     
- Misses        251      415      +164     
- Partials        8       37       +29

Impacted Files	Coverage Δ
lib/core/src/lib.rs	`0% <ø> (ø)`	⬆️
lib/core/src/display/rendering/htmlscene.rs	`0% <0%> (ø)`
lib/core/src/display/rendering/scene.rs	`0% <0%> (ø)`
lib/core/src/display/rendering/renderer.rs	`0% <0%> (ø)`
lib/system/web/src/lib.rs	`0% <0%> (ø)`	⬆️
lib/core/src/display/rendering/htmlrenderer.rs	`0% <0%> (ø)`
lib/core/src/display/world.rs	`0% <0%> (ø)`	⬆️
lib/core/src/display/rendering/htmlobject.rs	`0% <0%> (ø)`
lib/prelude/src/lib.rs	`100% <100%> (ø)`
lib/core/src/display/rendering/transform.rs	`46.15% <46.15%> (ø)`
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 389062e...bbd9cc2. Read the comment docs.

wdanilo · 2019-11-18T16:06:36Z

lib/core/Cargo.toml

@@ -5,7 +5,7 @@ authors = ["Wojciech Danilo <[email protected]>"]
 edition = "2018"

 [lib]
-crate-type = ["cdylib"]
+crate-type = ["rlib", "cdylib"]


Why we need rlib?

We need it to make the crate available in the tests.

See rust-lang/cargo#6659

wdanilo · 2019-11-18T16:09:15Z

lib/core/src/data/weakset.rs

+use std::collections::HashSet;
+use std::rc::{Rc, Weak};
+use std::hash::{Hash, Hasher};
+pub trait Hashable = Hash + PartialEq;


There should be 2 empty lines between imports and code + please, use the code separators:

// ================ // === Hashable === // ================

to divide parts of the file related to different entities. If there is a single entitiy, we use just a single separator on top of the file.

wdanilo · 2019-11-18T16:11:40Z

lib/core/src/data/weakset.rs

@@ -0,0 +1,113 @@
+use std::collections::HashSet;


Wait, after reviewing this file I realized that are not using WeakSet anywhere in you codebase, so why this file is committed? :D

Please read and reply to the reviews though.

My mistake! I considered using it, but then I relied on the existing opt_vec implementation. Should we keep it?

wdanilo · 2019-11-18T16:12:07Z

lib/core/src/data/weakset.rs

+    fn drop(&mut self) {
+        unsafe {
+            // no worries because it will happen synchronously
+            Rc::get_mut_unchecked(&mut self.owner).data.remove(&WeakElem { elem : Rc::downgrade(&self.elem) });


Please fix this and other lines that have >80 chars. Be sure to read our code style guide.

weakset.rs was removed as suggested

wdanilo · 2019-11-18T16:13:11Z

lib/core/src/data/weakset.rs

+impl<T : Hashable> Drop for WeakRef<T> {
+    fn drop(&mut self) {
+        unsafe {
+            // no worries because it will happen synchronously


Please use style guide regarding comments in code. Plus, this comment is not specific enough - comments regarding unsafe blocks should be really detailed and explain why we need such blocks.

weakset.rs was removed as suggested

wdanilo · 2019-11-18T16:31:23Z

lib/core/src/display/scene/htmlscene.rs

@@ -0,0 +1,50 @@
+use super::HTMLObject;


Format this file according to previous comments, make lines max 80 chars, make more vars, so you dont have multi-line exprs after refactoring, and this will be a very nice file! <3

A newer file formatted with cargo fmt should be available. Would you check it?

wdanilo · 2019-11-18T16:35:20Z

lib/core/src/display/scene/transform.rs

+pub struct Transform {
+    pub position:   Vector3<f32>,
+    pub quaternion: UnitQuaternion<f32>,
+    pub scale:      Vector3<f32>,


position and scale are the names of physical properties of objects. quaternion is a name of a math structure. I may be wrong, but I feel it's inconsistent here.

You are right. I renamed quaternion to rotation. I also renamed position to translation.

wdanilo · 2019-11-18T16:36:25Z

lib/core/src/display/scene/transform.rs

+// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// To comply with Threejs impl, we generate a Quaternion applying rotation in
+// the order: pitch -> roll -> yaw, instead of roll -> pitch -> yaw based on
+// https://github.com/mrdoob/three.js/blob/master/src/math/Quaternion.js#L199


A question - why we comply with Theejs impl? Based on this comment I understand it is not a standard way of handling it, right? Is it more performant?

There is no performance difference. It's just a convention and we can change it whenever we want.

As I was developing this functionality based on Three.js, I used it as a visual reference and I saw something was wrong. That's when I realized the rotation order was different.

wdanilo · 2019-11-18T16:37:18Z

lib/core/src/display/scene/transform.rs

+            self.quaternion.coords.w,
+        );
+        let (x2, y2, z2) = (x + x, y + y, z + z);
+        let (xx, xy, xz) = (x * x2, x * y2, x * z2);


very minor - alignment of ,

I think it was fixed by cargo fmt. Please check

wdanilo · 2019-11-18T16:37:35Z

lib/core/src/display/scene/transform.rs

+
+    // Note [Transform to Matrix4 composition]
+    // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    // based on https://github.com/mrdoob/three.js/blob/master/src/math/Matrix4.js#L732


This should be a doc comment

I left this as a source comment and provided a detailed explanation on how it works

wdanilo · 2019-11-19T01:25:27Z

lib/core/src/data/opt_vec.rs

@@ -47,6 +47,15 @@ impl<T> OptVec<T> {
        item.iter().for_each(|_| self.free_ixs.push(ix));
        item
    }
+
+    /// Gets the amount of items as Some(element)


I don't really understand the comment. What does as Some(element) mean?

Minor: no dot on the end of comment.

/// Returns the number of elements in the vector, also referred to as its 'length'.
Just like in Vec

wdanilo · 2019-11-19T01:25:33Z

lib/core/src/data/opt_vec.rs

+        self.items.len() - self.free_ixs.len()
+    }
+
+    pub fn is_empty(&self) -> bool {


No docs. I know this sounds stupid to add docs to is_empty method, but in order to have nice docs, let's do it. @iamrecursion tells us to do it :(

/// Returns true if vector contains no element.
Like in Vec too

wdanilo · 2019-11-19T01:27:08Z

lib/core/src/display/rendering/camera.rs

+use std::f32::consts::PI;
+
+/// A 3D camera representation with its own 3D
+// `Transform` and projection matrix


Wrong formatting:

// instead of ///

Please use full available width for doc comments (80 chars).

wdanilo · 2019-11-19T01:27:24Z

lib/core/src/display/rendering/camera.rs

+pub struct Camera {
+    #[shrinkwrap(main_field)]
+    pub object : Object,
+    pub projection : Matrix4<f32>,


What's the expected alignment here?

You mean to align ":"? If that's so, it's fixed.

wdanilo · 2019-11-19T01:31:44Z

lib/core/src/display/rendering/camera.rs

+    /// # Arguments
+    /// fov - Field of View in degrees
+    /// aspect - Aspect ratio of the screen
+    /// znear - Near distance clipping
+    /// zfar - Far distance clipping


Few comments here:

The rust-way of formatting doc-comments is that you put * before args (see https://doc.rust-lang.org/rust-by-example/meta/doc.html).

We tend NOT to comment the args to functions, as their names should be descriptive enough not to do so. In fact, comments should realy contain not-obvious information. I know sometimes its hard, but often it's possible and valuable. We should try to keep comments short and informative. Listing arguments does make sense very rarely.

I agree that obvious naming is usually better than documentation. I will rename them to

field_of_view_degrees: f32, aspect_ratio: f32, near_clipping: f32, far_clipping: f32

Much better!

wdanilo · 2019-11-19T02:02:26Z

lib/core/src/display/rendering/transform.rs

+{
+    let (s1, c1): (f32, f32) = (roll * 0.5 as f32).sin_cos();
+    let (s2, c2): (f32, f32) = (pitch * 0.5 as f32).sin_cos();
+    let (s3, c3): (f32, f32) = (yaw * 0.5 as f32).sin_cos();


very minor, but this code would look nicer with * aligned

wdanilo · 2019-11-19T02:04:08Z

lib/core/src/display/rendering/transform.rs

+        Self {
+            translation : Vector3::new(0.0, 0.0, 0.0),
+            rotation :    UnitQuaternion::identity(),
+            scale :       Vector3::new(1.0, 1.0, 1.0),
+        }


You can use 1-line exprs here to make it more clean and even shorter:

let translation = Vector3::new(0.0, 0.0, 0.0); let rotation = UnitQuaternion::identity(); let scale = Vector3::new(1.0, 1.0, 1.0); Self { translation, rotation, scale }

wdanilo · 2019-11-19T02:05:45Z

lib/core/src/lib.rs

@@ -10,6 +10,9 @@
 // === Module Structure Reexport ===
 // =================================

+#[macro_use]
+extern crate shrinkwraprs;


this is old rust style. use use instead. Or just import our Prelude instead :D

wdanilo · 2019-11-19T02:06:43Z

lib/system/web/src/lib.rs

+pub fn dyn_into<T, U>(obj : T) -> Result<U>
+where T : wasm_bindgen::JsCast + Debug,
+      U : wasm_bindgen::JsCast
+{


open brace goes to previous line

wdanilo · 2019-11-19T02:07:13Z

lib/system/web/src/lib.rs

+        self.set_attribute(name, value)
+            .unwrap_or_else(|_|
+                panic!("Failed to set attribute \"{}\" = \"{}\" on \"{:?}\"",
+                        name,
+                        value,
+                        self
+                )
+            );


Ouch :O
Single line exprs to the rescue!

wdanilo · 2019-11-20T06:36:54Z

lib/core/src/display/rendering/htmlrenderer.rs

+use crate::system::web::StyleSetter;
+use crate::prelude::*;
+
+fn eps(value: f32) -> f32 {


This definitely needs docs. I assume nobody who has not coded GL engines would understand why its needed.

wdanilo · 2019-11-20T06:43:45Z

lib/core/src/display/rendering/htmlrenderer.rs

+    pub fn render(&self, camera: &mut Camera, scene: &HTMLScene) {
+        let half_d = scene.get_dimensions() / 2.0;
+        // Note [znear from projection matrix]
+        let expr = camera.projection[(1, 1)]; // expr = 2.0 * near / (height)


This hardcoded (1,1) should have a dedicated method or something. It's not descriptive enough what it really does.

I really don't know how to simply describe it without explaining how a projection matrix is composed.

wdanilo · 2019-11-20T06:45:04Z

lib/core/src/display/rendering/htmlrenderer.rs

+        // Negating the second column to invert Y.
+        // Equivalent to scaling by (1.0, -1.0, 1.0).
+        transform.row_part_mut(1, 4).iter_mut().for_each(|a| *a = -*a);


instead of writing comment + code which is hard to understand, what about refactoring it to a function with a nice name like invert_y? :)

wdanilo · 2019-11-20T06:47:43Z

lib/core/src/display/rendering/transform.rs

+        let acc = format!("matrix3d({}", item);
+        let mut ret = iter.fold(acc, |acc, item| format!("{}, {}", acc, item));
+        ret.push(')');


This is ugly. push(')') is hacky. First concatenate children, then do format!("matrix3d({})", ...) instead.

HTML CSS3D Renderer implementation Original commit: enso-org/ide@81ae637

notdanilo added 7 commits November 13, 2019 16:59

HTMLScene and HTMLObject implementation

9f1b1cc

Added HTMLRenderer and Camera objects

a2611f4

Adding rlib as crate-type to make basegl available in tests

2d0665b

HTMLRenderer tests and code cleanup

6dcbb7a

Fixed transformations and adapted Quaternion euler transformation ord…

851a45a

…er from XYZ to YXZ (like threejs)

Added knowledge comments

e215b5f

Recovering original lib.rs (with world demo)

afba7cc

notdanilo added Category: Renderer Category: BaseGL The BaseGL library Difficulty: Core Contributor Should only be attempted by a core contributor Priority: High Should be scheduled as soon as possible Type: Enhancement An enhancement to the current state of Enso IDE labels Nov 18, 2019

notdanilo added this to the Sprint 2019-11-11 milestone Nov 18, 2019

Added shrinkwraprs

0d55dc5

notdanilo added 3 commits November 18, 2019 12:38

Formatted files

d545799

Improved opt_vec test coverage

e079756

Fixes based on clippy and rustfmt

2b32031

notdanilo changed the title ~~Wip/dg/htmlrenderer~~ HTML Rendering System Nov 18, 2019

wdanilo reviewed Nov 18, 2019

View reviewed changes

notdanilo added 5 commits November 18, 2019 14:40

Remove unused WeakSet :)

31cc11c

Added documentation

e6afdb2

Improved HTMLObject::from_html_string and beautified the code fmt

65a44d3

Created StyleSetter and AttributeSetter to deal with panics

9539c3e

Fixed code chars limits

7e5b28b

wdanilo reviewed Nov 19, 2019

View reviewed changes

notdanilo added 2 commits November 20, 2019 03:32

Matrix code cleanup and more code style guidelines fixes

cc530fa

More alignment fixes

708dabe

wdanilo reviewed Nov 20, 2019

View reviewed changes

notdanilo and others added 10 commits November 20, 2019 04:22

Some more fixes and refactoring

a035a5b

Refactored IntoCSSMatrix

0f9e501

Refactoring

1d668e4

Removed examples/02-html

ae97325

More refactoring

2765fce

Derived rendering structs from Debug

71e19fe

Refactoring (#50)

37e5331

More refactoring

f0b9545

Comments on HTMLRenderer tests

de83c22

Removed unwanted console_log

3331f84

wdanilo approved these changes Nov 22, 2019

View reviewed changes

notdanilo added 4 commits November 21, 2019 22:06

Recovered missed css matrix3d code

102d9d0

Added attribute to invalid doc codes

6e7d5f2

Clippy lints fixes

a61544a

Fixed function access

bbd9cc2

wdanilo merged commit 81ae637 into master Nov 22, 2019

mwu-tow pushed a commit to enso-org/enso that referenced this pull request Oct 30, 2021

HTML Rendering System (enso-org/ide#48)

23a0b27

HTML CSS3D Renderer implementation Original commit: enso-org/ide@81ae637

HTML Rendering System #48

HTML Rendering System #48

Uh oh!

Conversation

notdanilo commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Description

Important Notes

Checklist

Uh oh!

codecov-io commented Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

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!

notdanilo commented Nov 18, 2019 •

edited

Loading

codecov-io commented Nov 18, 2019 •

edited

Loading