Allowing EnsoGL mouse to interact with more than 4096 sprites at the same time. #3351

wdanilo · 2022-03-20T18:37:34Z

[ci no chfangelog needed]

Pull Request Description

This PR implements Enso mouse system should allow working with more than 4096 objects.. Until now, all sprites were distinguished by two values, symbol_id and instance_id. These values were encoded in an off-screen texture, on 12 bits each. This PR introduces a new abstraction, GlobalInstanceId, which unifies symbol_id and instance_id for the purpose of mouse target discovery. Both symbol_id and instance_id still exist, because they are usable in other parts of the codebase. Moreover, this PR introduces a few improvements:

It introduces a few helper structures, allowing for easier custom drop management of display objects and attributes.
It introduces a new derive macro NoCloneBecauseOfCustomDrop, which prevents the structure from implementing Clone and checks if the structure implements custom Drop behavior.
It abstracts part of Sprite implementation to a new concept SymbolInstance.
It implements shared2 macro, which is similar to shared, but unlike shared it is understandable by IntelliJ. However, shared2 does not support all constructs yet.
It extends EnsoGL renderer to support integer vertex input attributes.
It implements new debug utilities for generated shaders debugging and improves error messages.
It implements a new #[entry_point] macro, which simplifies entry point definition.
It implements ID overflow checking and reporting for GLSL shaders (more than 16M objects created).
It fixes the long-standing bug that symbols mouse down/up was not recorded. It is recorded correctly now.

FIXMES

Please note that one FIXME is left in the lib/rust/ensogl/core/src/debug/stats.rs file and should be investigated in the future. The cause is not introduced by this PR and because it requires some additional investigation time, it's not covered here.

Testing

⚠️⚠️⚠️ As this code changes a lot of basic rendering / logical concepts, it has to be heavily tested before being merged to develop ⚠️⚠️⚠️

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the Scala, Java, and Rust style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

MichaelMauderer · 2022-03-25T09:15:54Z

lib/rust/ensogl/core/src/display/object/class.rs

+// === UnsetParentOnDrop ===
+// =========================
+
+/// Wrapper that unsets parent of a display object when dropped.


Maybe there should be a special note that this does not imply "taking ownership" of the Instance (as they are clone_ref able, and there is no guarantee, that there are no other instances that will be alive and usable after this is dropped.

I'm mentioning this, as this at first glance looks like a pattern where some Rust object is put in a wrapper that makes guarantees about the inner object, but it is not. It is purely a utility to call a method on the inner object once this UnsetParentOnDrop sentinel is dropped.

MichaelMauderer · 2022-03-25T09:18:49Z

lib/rust/ensogl/core/src/display/scene.rs

+    /// Decode the [`PointerTarget`] from an RGBA value. If alpha is set to 0, the result will be
+    /// background. In case alpha is 255, the result will be decoded based on the first 3 bytes,
+    /// which allows for storing up to 16 581 375 unique IDs.
+    fn decode_from_rgba(v: Vector4<u32>) -> Result<Self, DecodeError> {


Some additional description about how the encoding and decoding is done should be in this doc. Just to ensure it is clear in which order the R, G, and B values are used and that there is no trickery with the endianness.

MichaelMauderer · 2022-03-25T09:21:02Z

lib/rust/ensogl/core/src/display/scene.rs

            }
        }
    }
-
-    #[test]


The new encoding should still be tested.

Comment from call: let's add docs to the decode line pointing to GLSL impl instead

MichaelMauderer · 2022-03-25T09:35:39Z

lib/rust/ensogl/core/src/system/gpu/data/buffer.rs

            let item_byte_size = T::item_gpu_byte_size() as i32;
            let item_type      = T::item_gl_enum().into();
            let rows           = T::rows() as i32;
            let cols           = T::cols() as i32;
            let col_byte_size  = item_byte_size * rows;
            let stride         = col_byte_size  * cols;
-            let normalize      = false;


Why is this renamed? normalize seems much more explicit.

MichaelMauderer · 2022-03-25T09:55:19Z

lib/rust/web/src/lib.rs

@@ -359,6 +359,14 @@ ops! { ReflectOps for Reflect
        /// [`get_nested`] to learn more.
        fn get_nested_object(target: &JsValue, keys: &[&str]) -> Result<Object, JsValue>;

+        /// Get the nested value of the provided object and cast it to [`Object`]. In case the


Should this be the same doc as below?

MichaelMauderer · 2022-03-25T10:01:12Z

lib/rust/ensogl/core/src/system/gpu/data/attribute.rs

+#[derive(Clone, CloneRef)]
+#[clone_ref(bound = "T:CloneRef")]
+pub struct EraseOnLastDrop<T: Erase> {
+    elem:  T,


Why does this need to store a second naked version of the element instead of just the Wrapped one?

Performance reasons, however, this code was not longer used and it was removed.

lib/rust/ensogl/core/src/display/symbol/gpu/shader/builder.rs

lib/rust/ensogl/core/src/display/symbol/gpu.rs

kazcw · 2022-03-25T14:29:50Z

lib/rust/ensogl/core/src/display/symbol/gpu.rs

+    GlobalInstanceId(u32);
+}
+
+shared2! { GlobalInstanceIdProvider


This data could be static--because this is a truly global object (it would be wrong for more than one instance of it ever to exist), it feels weird to me that objects have fields identifying an instance of it.

It is possible for multiple GlobalInstanceProviders to exist – every scene contains one. Right now, we have one scene everywhere, but our World supports having multiple scenes. Each scene is connected to a separate WebGL canvas (and separate WebGL context), thus has separate symbol buffers and separate instance counters.

That makes sense. The names or at least the docs could be more clear that these are actually scene-scoped, to ensure we avoid confusion when we have multiple scenes.

kazcw · 2022-03-25T17:53:08Z

lib/rust/ensogl/core/src/display/object/class.rs

@@ -1188,6 +1188,32 @@ impl<Host> Object<Host> for Any<Host> {



+// =========================


This UnsetParentOnDrop is cleaner than what it replaces, but the need for it it suggests a complex lifetime dynamic I don't fully understand.

Why is it that Sprite's display Instance needs to be unparented if the Sprite is dropped, but an Instance for any other object doesn't need this?

This is an interesting question. You know, right now all things that you see on the screen are Sprites, so this works for everything we have. We could theoretically use this pattern in every object (like a node, that contains a lot of sprites), but I'm not yet sure what benefits it would give us. But this is an interesting potential change (definitely not part of this PR as it could be really big). CC @farmaazon, what do you think?

lib/rust/ensogl/core/src/display/shape/primitive/glsl/math.glsl

… at the same time. (#3351)" This reverts commit 546c333.

… at the same time. (#3351)" (#3368) This reverts commit 546c333.

… sprites at the same time. (#3351)" (#3368)" This reverts commit 7152e0d.

wdanilo added 3 commits March 20, 2022 08:49

wip

3d10b0d

Improving the shared macro

7a16067

Adding global id param to symbols

cfa72a2

MichaelMauderer mentioned this pull request Mar 21, 2022

Introduce define_id!; use it to obtain unique IDs safely #3350

Closed

4 tasks

kazcw mentioned this pull request Mar 23, 2022

Profiling: intervals tool and devtools-file generator #3359

Merged

4 tasks

wdanilo added 3 commits March 24, 2022 14:59

wip

f00deee

Improved error reporting

e8183c9

Improved shader gen

f1f8ba6

wdanilo force-pushed the wip/wd/ensogl-object-count branch from 39c1e42 to f1f8ba6 Compare March 24, 2022 14:09

wdanilo added 6 commits March 24, 2022 16:06

Adding support for integer attributes in shaders

a5fbd41

Propagating id information and new test scene

3bd377c

First working version

afcde54

Cleaning

52ed0f1

Cleaning

be408e3

Cleaning

c58122e

wdanilo changed the title ~~[WIP] Allowing EnsoGL mouse to interact with more than 4096 sprites at the same time.~~ Allowing EnsoGL mouse to interact with more than 4096 sprites at the same time. Mar 25, 2022

wdanilo requested review from MichaelMauderer, farmaazon, mwu-tow and kazcw March 25, 2022 04:55

wdanilo marked this pull request as ready for review March 25, 2022 04:56

wdanilo requested a review from 4e6 as a code owner March 25, 2022 04:56

Cleaning

1a73fad

MichaelMauderer reviewed Mar 25, 2022

View reviewed changes

kazcw reviewed Mar 25, 2022

View reviewed changes

wdanilo added 5 commits March 26, 2022 01:12

Linting and cleaning

7ccda60

Fixing mouse press and release handlers in scene, refactoring

43db115

Refactoring MouseTarget

1e62d0e

Refactoring

081fb4a

Code formatting

c263f0b

wdanilo added 5 commits March 28, 2022 07:35

Applying review

1004998

Merge branch 'develop' into wip/wd/ensogl-object-count

7c8a935

Applying code review

2f734a2

Refactoring

8a7db06

Refactoring

9aff3c3

wdanilo mentioned this pull request Mar 28, 2022

The mouse_down event is not delivered to shapes if not under project view #2899

Closed

kazcw reviewed Mar 28, 2022

View reviewed changes

lib/rust/ensogl/core/src/display/shape/primitive/glsl/math.glsl Outdated Show resolved Hide resolved

wdanilo added 2 commits March 29, 2022 03:14

Applying review

a851a48

Merge branch 'develop' into wip/wd/ensogl-object-count

2e2b400

wdanilo merged commit 546c333 into develop Mar 29, 2022

wdanilo deleted the wip/wd/ensogl-object-count branch March 29, 2022 02:15

akavel added a commit that referenced this pull request Mar 29, 2022

Revert "Allowing EnsoGL mouse to interact with more than 4096 sprites…

fe93ad9

… at the same time. (#3351)" This reverts commit 546c333.

akavel mentioned this pull request Mar 29, 2022

Revert "Allowing EnsoGL mouse to interact with more than 4096 sprites… #3368

Merged

4 tasks

sylwiabr pushed a commit that referenced this pull request Mar 29, 2022

Revert "Allowing EnsoGL mouse to interact with more than 4096 sprites…

7152e0d

… at the same time. (#3351)" (#3368) This reverts commit 546c333.

wdanilo added a commit that referenced this pull request Mar 29, 2022

Revert "Revert "Allowing EnsoGL mouse to interact with more than 4096…

c66126c

… sprites at the same time. (#3351)" (#3368)" This reverts commit 7152e0d.

wdanilo added a commit that referenced this pull request Mar 29, 2022

Revert "Revert "Allowing EnsoGL mouse to interact with more than 4096…

e5c2578

… sprites at the same time. (#3351)" (#3368)" This reverts commit 7152e0d.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowing EnsoGL mouse to interact with more than 4096 sprites at the same time. #3351

Allowing EnsoGL mouse to interact with more than 4096 sprites at the same time. #3351

wdanilo commented Mar 20, 2022 •

edited

Loading

MichaelMauderer Mar 25, 2022

MichaelMauderer Mar 25, 2022

MichaelMauderer Mar 25, 2022

wdanilo Mar 25, 2022

MichaelMauderer Mar 25, 2022

MichaelMauderer Mar 25, 2022

MichaelMauderer Mar 25, 2022

wdanilo Mar 28, 2022

kazcw Mar 25, 2022

wdanilo Mar 28, 2022

kazcw Mar 28, 2022

kazcw Mar 25, 2022 •

edited

Loading

wdanilo Mar 28, 2022

                           }
                       }
                   }
-                  #[test]

		@@ -1188,6 +1188,32 @@ impl<Host> Object<Host> for Any<Host> {



		// =========================

Allowing EnsoGL mouse to interact with more than 4096 sprites at the same time. #3351

Allowing EnsoGL mouse to interact with more than 4096 sprites at the same time. #3351

Conversation

wdanilo commented Mar 20, 2022 • edited Loading

Pull Request Description

FIXMES

Testing

Checklist

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

kazcw Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdanilo commented Mar 20, 2022 •

edited

Loading

kazcw Mar 25, 2022 •

edited

Loading