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

Add a method to redraw wave form #1127

Closed
tf opened this issue Jun 14, 2017 · 17 comments
Closed

Add a method to redraw wave form #1127

tf opened this issue Jun 14, 2017 · 17 comments

Comments

@tf
Copy link
Contributor

tf commented Jun 14, 2017

It would be nice if there was an API method to manually trigger redrawing the wave form. I mainly see two use cases:

Responsiveness Projects like react-wavesurfer already use drawBuffer to redraw the wave form once the container size has changed. As described in this issue this causes problems since drawBuffer appears not to clear the canvas before redrawing causing the waveform to look less crisp once the viewport is resized.

Updating options I'd also like an official way to change options like waveColor and progressColor
for an already initialized wavesurfer player. This is possible already now by calling:

        wavesurfer.params.waveColor = nextProps.options.waveColor;
        wavesurfer.drawer.clearWave();
        wavesurfer.redrawBuffer();

It would be nice if there was an official way to achieve this. Something like a redraw method could be a step in this direction. Still, I am not sure if mutating wavesurfer.params should also be replaced by a method call along the lines of the existing toggleInteraction/toggleScroll, i.e. setWaveColor.

I'd be happty to sumit a PR if we can agree on a direction.

@agamemnus
Copy link
Contributor

My PR (here: #1133) lets you put elements under the waveform, so you don't need to redraw it. FYI.

@tf
Copy link
Contributor Author

tf commented Jun 16, 2017

@agamemnus interesting idea. Thanks for the heads up. This would still only work if the wave is placed on a background that matches the color used for the "inverted mask" in the canvas, right? In my case there is a large background image behind the wave.

@tf tf changed the title Suggestion: Add a method to redraw wave form Add a method to redraw wave form Jun 16, 2017
@agamemnus
Copy link
Contributor

Well, that is true. But you could also modify it to make the mask an image instead of a colored rectangle...

@tf
Copy link
Contributor Author

tf commented Jun 16, 2017

@agamemnus I'm sure there are cases where this would work, but in my case the background image is really dynamic and unrelated to the waveform. So I think the suggested API additions still make sense.

@katspaugh Looking a bit further, I found that calling drawBuffer originally worked. Adapting the minimal example that I posted before to use 1.2.8 makes it work fine.

The breaking change is #909 in 1.3.0, more precisely this change. Before, the call chain looked like this:

The change in #909, makes setWidth short circuit if the width has not changed causing the canvas not be cleared.

In the light of this new insights, I'd argue that drawBuffer already is the method I was looking for and it should be fixed to clear the canvas again.

@agamemnus
Copy link
Contributor

agamemnus commented Jun 16, 2017 via email

@agamemnus
Copy link
Contributor

@tf: A little lost but why drawbuffer and not drawPeaks?

@katspaugh: Would it make sense to put WaveSurfer.util.frame into drawBuffer instead of drawBars/drawWave? Also, maybe the channel splitting could be done there too? What do you think?

Also, instead of setWaveColor, maybe getters/setters might be more interesting... some of my recent PRs for example link up cursorColor and the actual cursor color (where the cursor is now a DOM element).

@agamemnus
Copy link
Contributor

agamemnus commented Jun 17, 2017

@katspaugh --> #1132.

tf added a commit to tf/wavesurfer.js that referenced this issue Jun 19, 2017
refs katspaugh#1127

Before katspaugh#909, calling `drawBuffer` to redraw the wave invoked
`drawer.drawPeaks` which in turn invoked `drawer.setWidth` which
always caused the canvas to be cleared as a side effect of
`drawer.updateSize`.

In katspaugh#909 `setWidth` was changed to short circuit if the width did not
change. This now causes the waveform to be redrawn on top of the
previous rendition, making the edges of the wave appear less crisp.

This change makes `setWidth`/`setHeight` return a boolean to indicate
whether changes were needed. If not, `drawer.clearWave` is called
manually. So we make sure that the previous wave is always cleared,
but do not perform the possibly performance intensive task of clearing
the canvas twice if it already happened as a side effect of
`setWidth`.
@tf
Copy link
Contributor Author

tf commented Jun 20, 2017

@agamemnus drawPeaks is a method on drawer and should therefore not be part of the public API, I guess. drawBuffer calls drawPeaks, though.

@katspaugh I've created a PR (#1144) with a possible solution for the redrawing bug.

Moreover, in #1145 I've introduces getters and setters for height, waveColor and progressColor following the same API schema already used for methods like getPlaybackRate/setPlaybackRate.

@tf
Copy link
Contributor Author

tf commented Jun 21, 2017

So in the end I did now send a PR that introduces a new refresh method since only calling drawBuffer does not update the progress canvas width correctly if the container width changed.

@agamemnus
Copy link
Contributor

agamemnus commented Jun 22, 2017

Oh. I haven't tried changing the size of the container.

If you use a function-based get/set, instead of overwriting the getter and setter, the params are treated as throwaway variables, or they are not in sync. With my method, they are in sync.

I also feel like refresh might be too vague. Why not make drawBuffer update the progress canvas width correctly if the container width changed?

Here's my current code: (it doesn't update the size per the container size, I think)
https://github.com/agamemnus/wavesurfer.js/blob/master/dist/wavesurfer.js

Try something like wavesurfer.params.progressColor = 'green', or wavesurfer.params.progressBackgroundColor = 'blue' etc. You can even change the style by updating the barWidth to either undefined or some number...

@tf
Copy link
Contributor Author

tf commented Jun 22, 2017

drawBuffer is used in a lot of different places. I'd be afraid to introduce performance regressions by also updating canvas width in all of those cases.

@agamemnus
Copy link
Contributor

If the container width changed, then you would have no choice, though.

katspaugh pushed a commit that referenced this issue Jul 2, 2017
refs #1127

Before #909, calling `drawBuffer` to redraw the wave invoked
`drawer.drawPeaks` which in turn invoked `drawer.setWidth` which
always caused the canvas to be cleared as a side effect of
`drawer.updateSize`.

In #909 `setWidth` was changed to short circuit if the width did not
change. This now causes the waveform to be redrawn on top of the
previous rendition, making the edges of the wave appear less crisp.

This change makes `setWidth`/`setHeight` return a boolean to indicate
whether changes were needed. If not, `drawer.clearWave` is called
manually. So we make sure that the previous wave is always cleared,
but do not perform the possibly performance intensive task of clearing
the canvas twice if it already happened as a side effect of
`setWidth`.
mspae pushed a commit to mspae/wavesurfer.js that referenced this issue Aug 18, 2017
refs katspaugh#1127

Before katspaugh#909, calling `drawBuffer` to redraw the wave invoked
`drawer.drawPeaks` which in turn invoked `drawer.setWidth` which
always caused the canvas to be cleared as a side effect of
`drawer.updateSize`.

In katspaugh#909 `setWidth` was changed to short circuit if the width did not
change. This now causes the waveform to be redrawn on top of the
previous rendition, making the edges of the wave appear less crisp.

This change makes `setWidth`/`setHeight` return a boolean to indicate
whether changes were needed. If not, `drawer.clearWave` is called
manually. So we make sure that the previous wave is always cleared,
but do not perform the possibly performance intensive task of clearing
the canvas twice if it already happened as a side effect of
`setWidth`.
mspae pushed a commit that referenced this issue Aug 19, 2017
refs #1127

Before #909, calling `drawBuffer` to redraw the wave invoked
`drawer.drawPeaks` which in turn invoked `drawer.setWidth` which
always caused the canvas to be cleared as a side effect of
`drawer.updateSize`.

In #909 `setWidth` was changed to short circuit if the width did not
change. This now causes the waveform to be redrawn on top of the
previous rendition, making the edges of the wave appear less crisp.

This change makes `setWidth`/`setHeight` return a boolean to indicate
whether changes were needed. If not, `drawer.clearWave` is called
manually. So we make sure that the previous wave is always cleared,
but do not perform the possibly performance intensive task of clearing
the canvas twice if it already happened as a side effect of
`setWidth`.
@aleksnc
Copy link

aleksnc commented Aug 20, 2017

Awfully know English. Sorry.

How to draw a wave after the team "create()"?

I have the code in the project

https://github.com/aleksnc/TuneTone

function initSound() {
var Soundlenght = SoundInit.length;

if (Soundlenght == 0) {
    $('.Singleresult').css(
        {
            'height': 0,
            'overflow': 'hidden'
        });
}


$.each(SoundData, function (i, item) {


    if (Soundlenght == 0) {
        SoundInit[i] = WaveSurfer.create({
            container: '#waveform' + item.id,
            waveColor: '#828082',
            progressColor: '#3bd9a3',
            height: 80
        });

        SoundInit[i].load('music/' + item.realTitle);

    } else {
     ???? <- what have I to write?
    }

    AddInfoMusic(i, item);


});

}

my course of action.

  1. open "http://localhost:3000/SearchResult.html"
    everything is drawn.
  2. click on "play"
    music plays
  3. click on the link (picture, artist name, or song title)
    the music continues to play without interruption (it's good).
    the wave is not drawn (this is bad)
  4. press "F5" and the waves drew? because there was no team."create()"

Thank you!)

@mspae
Copy link
Contributor

mspae commented Aug 20, 2017

@aleksnc What version are you using? There is a bug, which caused the waveform to not be drawn introduced in #1191 which is fixed once #1193 is merged. Maybe related?

Please open a new issue with your question, otherwise it's too difficult to follow if multiple issues are talked about in one thread. Thank you!

@mspae
Copy link
Contributor

mspae commented Sep 2, 2017

Since the responsive feature is now part of core can we close this issue @tf @agamemnus ?

@agamemnus
Copy link
Contributor

I see. Probably totally different from what I did, though...

@tf
Copy link
Contributor Author

tf commented Nov 3, 2017

@mspae I agree that this issue is no longer actionable.

@tf tf closed this as completed Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants