Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wip/wdanilo/widgets 182746060 #3678

Merged
merged 242 commits into from
Oct 4, 2022
Merged

Wip/wdanilo/widgets 182746060 #3678

merged 242 commits into from
Oct 4, 2022

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented Sep 1, 2022

[ci no changelog needed]

Pull Request Description

This PR introduces several big changes in crucial areas, so it is recommended to every developer carefully read the below changelog.

Global changes (incl. prelude).

  1. Tracing is our default logger now. Prelude exports warn!, debug!, etc. from Tracing now. A lot of APIs were refactored to support it.
  2. There is a new struct VecIndexedBy<T, I = usize, A: Allocator = std::alloc::Global> which is like Vec but uses I as indexes (instead of usize). This allows for building better APIs. The NonEmptyVec was modified to support it as well.3.
  3. New std::Range extensions were added. See lib/rust/prelude/src/range.rs to learn more.
  4. There is a new define_not_same_trait macro exported in the prelude. Its implementation is simple:
macro_rules! define_not_same_trait {
    () => {
        auto trait NotSame {}
        impl<T> !NotSame for (T, T) {}
    };
}

It can be used to disambiguate trait impls. For example, this is not working in Rust:

struct A<T>{t:T}
impl<T, S> From<A<T>> for S where T:Into<S> {...}

because it conflicts with std-defined impl<T> From<T> for T {...}. Now, you can define it as:

define_not_same_trait!();
struct A<T>{t:T}
impl<T, S> From<A<T>> for S where T:Into<S>, (A<T>,S): NotSame {...}

Somehow, it does not work if the trait is just defined in the prelude and it has to be defined locally.

FRP changes.

Consider the following definition:

ensogl_core::define_endpoints! {
    Input {
        set_property (TextRange, Option<style::Property>)
    }
}

To call it, you can provide arguments with the following types (which will be auto-converted to the expected types):

Operation New behavior Old behavior
(TextRange, Option<style::Property>) ✅ Supported. ✅ Not supported.
(&TextRange, Option<&style::Property>) ✅ Supported. ✅ Not supported.
(&TextRange, style::Property) ✅ Supported. ✅ Not supported.
(Range<Ubytes>, style::Property) ✅ Supported, where there is impl From<Range<Ubytes>> for TextRange. ❌ Not supported.
TextRange, color::Red) ✅ Supported, where there is impl From<color::Red> for style::Property (automatic conversion from color::Red to Option<style::Property>. ❌ Not supported.

Changes the animation framework.

In the past, each animation::Loop was a separate JS animation loop (constructed with requestAnimationLoop under the hood). The problem with this approach is that we do not control the order of the callbacks running and we do not know when they finish. Let's consider this use case - we want to know what is the width of the text area. It can contain multiple cursors in multiple lines, each with animation after glyph insertion. This makes multiple animations of possibly multiple lines that can change the text area width. With the new animation::Loop implementation it is possible to run all the animations and then, after they are updated (per-frame), check which line was the longest.

Changes in text rendering

Basically, text rendering performance is drastically improved and text now supports any Unicode left-to-right glyphs properly. The text rendering performance will be further improved in the next PR, which will introduce the rendering of multiple text areas in a single render pass.

Operation New behavior Old behavior
Setting variable fonts. ✅ Supported. ❌ Not supported.
Making glyphs bold or italic. ✅ Supported. ❌ Not supported. It is really complex because bold and italic styles may be defined in separate .ttf files, and thus, the text area needs to be able to mix different definitions into one abstraction.
Changing glyph color or glyph sdf-width. ✅ Changing GPU properties of glyphs only with nice built-in animations. Currently, the demo scene redraws the text once during startup. In the old implementation, it was 20+ redraws. ❌ Relayouting glyphs and redrawing all lines. Animations were not possible because redrawing was so costly.
Changing glyph complex properties as size, bold, and style (e.g. italics). ✅ Supported. Including proper cursor and selection management in lines using multiple styles. ❌ Not supported.
Changing default text properties, like its default color. ✅ Supported. ❌ Partially supported, but what was supported was broken.
Incremental text edits. ✅ Supported. After text editing, only a minimal portion of a text is re-drawn. All other parts are re-used, which works nicely with animations. ❌ Not supported. Each edit required a full-text redraw.
Modifying text properties, like increasing the text size in a given range. ✅ Supported. ❌ Not supported.
User-friendly API for working with text. ✅ Available. You can now change glyph properties by using any metric of lines, columns, byte offsets, current selections, etc. ❌ Byte-offset only API, very easy to break.
Support for UTF-8 glyphs. 🟡 Supported only in left-to-right layout. No emoji support. For now, this is enough and this is a very complex thing. For example, we want to redraw only part of a long line of text after inserting a glyph. I want to know what part of the text should be redrawn. Let's consider the input அட0. If we insert after , then we should get ட் instead of . Moreover, the ட் glyph can have different x-axis dimensions than the one. All of this is now supported. ❌ Not supported. It used to crash the IDE.
Support for proper glyph positions. ✅ Supported. Both x-axis and y-axis properties are now computed using a state-of-the-art library rustybuzz. ❌ Not supported. We had a custom, limited implementation and hardcoded line height.
Using safe types ✅ Yes. The implementation now uses new types, such as UBytes, or ViewLine in order to indicate that the value can not be negative or is used to describe the scrolled-line index, respectively. This allowed catching multiple bugs in the old implementation. ❌ No. Using usize in many places, often wrongly.
Support for scrolled text. 🟡 Supported viewing selected range of lines. There are no scrollbars built in yet, they will probably be added in the future. ❌ Broken supported. Support was there, but was not working properly at all.
Efficient rendering of long lines of glyphs. 🟡 Not supported. This is a hard topic. There is no simple way of discovering which glyphs to reshape/rerender after an edit and this needs to be handled in a very special way in the future. We do not see any solution for it yet, however, it should not cause any problems now. There is the possibility that what we do now is actually what we should do, which means, that after an edit happens, we are reshaping the whole line, computing the positions, and rendering only the needed glyphs. ❌ Not supported.
Glyph system as EnsoGL shape system. ❌ Not supported. Will be supported in the next PR. This will allow rendering all text areas with a single draw call and increase speed drastically. ❌ Not supported.

Fixed bugs

  1. Some time ago we introduced optimization to not render the scene if there is nothing moving. Unfortunately, this was not considering GPU-only animations (like cursor blinking) and was causing these animations to freeze until the mouse was being moved or other animations running. This PR fixes this issue by counting the number of currently running GPU animations and not refreshing the stage if the number drops to zero only.

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 ide build and ./run ide watch.

@wdanilo wdanilo force-pushed the wip/wdanilo/widgets-182746060 branch from d005648 to 2c3ea11 Compare September 1, 2022 17:52
Copy link
Contributor

@kazcw kazcw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't read everything yet; this is Part 1.

lib/rust/ensogl/core/Cargo.toml Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/animation/easing.rs Outdated Show resolved Hide resolved
lib/rust/ensogl/core/src/debug/monitor.rs Show resolved Hide resolved
lib/rust/prelude/src/range.rs Show resolved Hide resolved
lib/rust/prelude/src/range.rs Outdated Show resolved Hide resolved
lib/rust/web/src/lib.rs Outdated Show resolved Hide resolved
lib/rust/prelude/src/range.rs Show resolved Hide resolved
lib/rust/prelude/src/string.rs Show resolved Hide resolved
@wdanilo
Copy link
Member Author

wdanilo commented Oct 4, 2022

@kazcw thanks so much for the review. Good comments. Many of these fixmes were left for my next PR - they should be marked as such. Multiple other PRs are blocked by this PR and this is long overdue, so I'll merge it, I'll apply all these comments in the upcoming PR, and I'll ping you there. I hope this is OK?

@wdanilo
Copy link
Member Author

wdanilo commented Oct 4, 2022

@kazcw I applied these comments. Please make rest of the review and I'll apply it in the next PR.

@wdanilo wdanilo merged commit 61546a7 into develop Oct 4, 2022
@wdanilo wdanilo deleted the wip/wdanilo/widgets-182746060 branch October 4, 2022 02:51
Copy link
Contributor

@kazcw kazcw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 2/3; a few files left to look at tomorrow.

lib/rust/prelude/src/data/vec_indexed_by.rs Show resolved Hide resolved
lib/rust/prelude/src/data/vec_indexed_by.rs Show resolved Hide resolved
lib/rust/prelude/src/data/vec_indexed_by.rs Show resolved Hide resolved
lib/rust/ensogl/component/text/src/buffer.rs Show resolved Hide resolved
lib/rust/ensogl/component/text/src/buffer.rs Show resolved Hide resolved
Copy link
Contributor

@kazcw kazcw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part 3/3.

Besides the comments below, I've noticed that in a few places we have algorithms that are quadratic (or in one case, O(nm)) that we could be doing in loglinear time (or in one case, O(n+m)). That's fine as long as we don't need to handle huge numbers of visible lines, or huge numbers of simultaneous selections. I tried to quantify how far we could scale those factors without getting unacceptably slow, but cargo bench seems to be broken.

@wdanilo wdanilo restored the wip/wdanilo/widgets-182746060 branch October 6, 2022 21:05
@farmaazon farmaazon mentioned this pull request Oct 7, 2022
4 tasks
@wdanilo
Copy link
Member Author

wdanilo commented Oct 10, 2022

Part 3/3.

Besides the comments below, I've noticed that in a few places we have algorithms that are quadratic (or in one case, O(nm)) that we could be doing in loglinear time (or in one case, O(n+m)). That's fine as long as we don't need to handle huge numbers of visible lines, or huge numbers of simultaneous selections. I tried to quantify how far we could scale those factors without getting unacceptably slow, but cargo bench seems to be broken.

This is a very important observation - could you tell more about the places that you've found, please? I think we should fix them.

mergify bot pushed a commit that referenced this pull request Oct 10, 2022
There was a regression introduced by PR #3678 with text synchronization between IDE and the Engine. This PR fixes it.

It was reproducible as an error log about version mismatch in a case when the metadata becomes shorter (e.g. on removing node).

# Important Notes
[ci no changelog needed]
@kazcw
Copy link
Contributor

kazcw commented Oct 10, 2022

Part 3/3.
Besides the comments below, I've noticed that in a few places we have algorithms that are quadratic (or in one case, O(nm)) that we could be doing in loglinear time (or in one case, O(n+m)). That's fine as long as we don't need to handle huge numbers of visible lines, or huge numbers of simultaneous selections. I tried to quantify how far we could scale those factors without getting unacceptably slow, but cargo bench seems to be broken.

This is a very important observation - could you tell more about the places that you've found, please? I think we should fix them.

Most users of selection::Group::merge call it in a loop, which is quadratic under some conditions; we could do it quasilinearly by sorting the new inputs and merging them into the group in one pass.

This case in update_lines_after_change is quadratic-time, but it could be linear if we insert the new lines en masse.

mwu-tow pushed a commit that referenced this pull request Nov 30, 2022
Fix issues noted here: #3678 (comment)
- Time complexity of an operation during line-redrawing scaled quadratically with number of lines in a change; now linear.
- Time complexity of adding `n` selections to a group was `O(n^2)`. Now it is `O(n log n)`, even if the selections are added one by one.

Also fix a subtle bug I found in `Group::newest_mut`: It returned a mutable reference that allowed breaking the *sorted* invariant of the selection group. The new implementation moves the element to invalidated space before returning a reference (internally to `LazyInvariantVec`), so that if it is mutated it will be moved to its correct location.

### Important Notes

New APIs:
- `NonEmptyVec::extend_at` supports inserting a sequence of elements at a location, with asymptotically-better performance than a series of `insert`s. (This is a subset of the functionality of `Vec::splice`, a function which we can't safely offer for `NonEmptyVec`).
- `LazyInvariantVec` supports lazily-restoring an invariant on a vector. For an invariant such as *sorted* (or in this case, *sorted and merged*), this allows asymptotically-better performance than maintaining the invariant with each mutation.
@wdanilo wdanilo deleted the wip/wdanilo/widgets-182746060 branch February 21, 2023 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants