Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

use Snapshot API instead of cairo #64

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vixalien
Copy link
Owner

fix #61

cc @A6GibKm

@vixalien vixalien force-pushed the wip/vixalien/snapshots-direct branch from be9200c to fc366f9 Compare February 19, 2024 22:49
src/waveform.ts Outdated
ctx.moveTo(horizCenter, vertiCenter - height);
ctx.lineTo(horizCenter, vertiCenter + height);
ctx.stroke();
// Clip the snapshot to the widget area.
Copy link
Contributor

Choose a reason for hiding this comment

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

In general the more sensible approach is to draw inside this rectangle, this code implies that things will be drawn outside and can appear half-way offscreen.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You're correct. The reason is that the peaks are drawn "scaled up" by another 70%

Usually the waveform peaks are short (50-70%) of the widget height. I make them a bit larger so that there's not much empty space.

Should I stop clipping the peaks or should I instead clip the waveform lines before they are given their size is larger than widget.height?

Copy link
Contributor

@A6GibKm A6GibKm Feb 19, 2024

Choose a reason for hiding this comment

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

I am not familiar with the code, ideally one does the math and draws them at a size based on height in such a way that even after adding the position it does not go beyond the given size.

Without thinking too much, I would set the rectangle with height factor * height (factor goes from 0 to 1) and a y position given by (height - factor * height) / 2.0 so that it looks centered.

Note that you have not ensure everything passed to the snapshot API (e.g. values for a rectangle) is an integer if you want to avoid blurryness. (just use a floor() fn whenever you do a division or multiple by a random float like factor).

I will try to test or review this with more time in the future.

@A6GibKm
Copy link
Contributor

A6GibKm commented Feb 24, 2024

What about

diff --git a/src/waveform.ts b/src/waveform.ts
index 892696a..b0da70a 100644
--- a/src/waveform.ts
+++ b/src/waveform.ts
@@ -134,12 +134,6 @@ export class APWaveForm extends Gtk.Widget {
 
     const leftColor = this.safeLookupColor("accent_color");
 
-    // Clip the snapshot to the widget area.
-    // Turns out the DrawingArea was automatically doing that for us
-    snapshot.push_clip(
-      new Graphene.Rect({ size: new Graphene.Size({ width, height }) }),
-    );
-
     const indicator = new Graphene.Rect({
       origin: new Graphene.Point({ x: horizCenter, y: 0 }),
       size: new Graphene.Size({ width: LINE_WIDTH, height }),
@@ -169,7 +163,7 @@ export class APWaveForm extends Gtk.Widget {
 
       // only show 70% of the peaks. there are usually few peaks that are
       // over 70% high, and those get clipped so that not much space is empty
-      const line_height = Math.max(peak * height * 0.7, 1);
+      const line_height = Math.max(peak * height * 0.35, 1);
 
       const line = new Graphene.Rect({
         origin: new Graphene.Point({
@@ -188,8 +182,6 @@ export class APWaveForm extends Gtk.Widget {
 
       pointer += GUTTER;
     }
-
-    snapshot.pop();
   }
 
   set position(pos: number) {

The previous code had line_height = peak * height * 0.7, but the name is a bit misleading. This was not used as the height of the line, but rather the height from its center therefore one should have used height * 0.5 rather than height, assuming peak is in in the 0.0-1.0 range. With peak * height * 0.35 the maximum possible height of the entire line is height * 0.7 which fits in the square.

EDIT: Alternatively, use 0.7 as before, but define the rect as

      const line = new Graphene.Rect({
        origin: new Graphene.Point({
          x: pointer,
          y: vertiCenter - line_height / 2.0,
        }),
        size: new Graphene.Size({
          width: LINE_WIDTH,
          height: line_height,
        }),
      });

this will also ensure that the squares are actually 1x1 in their smallest size.

@A6GibKm
Copy link
Contributor

A6GibKm commented Feb 24, 2024

Another thing that you might consider is using rounded rect for the lines so that their edges are slightly rounded, but please ask designers first.

@vixalien
Copy link
Owner Author

vixalien commented Mar 2, 2024

Another thing that you might consider is using rounded rect for the lines so that their edges are slightly rounded.

I feel like doing this in a follow up MR would be a better idea. I'll implement your suggestion soon

@vixalien
Copy link
Owner Author

Your suggestion makes the line just halved. What I want is the graph to be 0.75 larger (because otherwise the peaks will be smaller) and if the peaks become too large, the should get clipped.

@vixalien
Copy link
Owner Author

FWIW, the original Decibels multiplied the waveform by 2. You can see this in the following screenshot comparing the new decibels to the old one.

image

@vixalien
Copy link
Owner Author

The recent commits ensure the waveform lines are inside the bounds without using clip rects.

@vixalien vixalien requested a review from A6GibKm March 11, 2024 21:43
@A6GibKm
Copy link
Contributor

A6GibKm commented Mar 11, 2024

Your suggestion makes the line just halved. What I want is the graph to be 0.75 larger (because otherwise the peaks will be smaller) and if the peaks become too large, the should get clipped.

I would suggest to draw a border of size width x height on the widget as a sanity check. Its important to get everything fitting inside the canvas.

@A6GibKm
Copy link
Contributor

A6GibKm commented Mar 11, 2024

Another thing, is that if it is too small, it is a better idea to give more space to the widget than to potentially go out of the canvas.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port Drawing from Cairo to GtkSnapshot API
2 participants