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

What to do with our waves-ui fork? #24

Open
cannam opened this issue May 18, 2017 · 7 comments
Open

What to do with our waves-ui fork? #24

cannam opened this issue May 18, 2017 · 7 comments

Comments

@cannam
Copy link

cannam commented May 18, 2017

Hello there -

For a small project here at the Centre for Digital Music, we have found ourselves making a number of modifications to Waves-UI during the past few months.

These currently reside in a repository here: https://github.com/piper-audio/waves-ui-piper

The README (and of course the commit history) outline the changes we've made, but the general thrust is that we wanted to speed up drawing and navigation of long waveform + annotation tracks, particularly in CenteredZoomState. We've also added a couple of other shape types (including a matrix/image shape for use by spectrograms) and made a few further modifications.

I'm posting this as an issue (for discussion) rather than a pull request (for merging) because I'm unsure how welcome this miscellaneous pile of changes would be for you. As our README notes, some of the changes are quite intrusive and may lead to incompatibilities for code written to use the original Waves-UI. They also add quite a bit of complexity in places. In some cases we have made changes that we needed for our application even where we weren't yet confident whether they represented good design.

I'd appreciate your thoughts on the right way to go ahead here - for example whether you would like to consider pulling these changes, or would find it preferable for this to remain as a separate repository at this point, or would like another way forward.

The application we've been working on that uses this library is not quite ready for launch yet, so another option might be to wait and take a look at how well (or badly) that application works. I've updated the online example code in the library to exploit the new additions, but that still isn't very much to go on.

Thanks for your views, and thank you for publishing this thoughtful library in the first place.

@b-ma
Copy link
Contributor

b-ma commented May 19, 2017

Hi @cannam

Good news ! I had an eye on your changes and so far, I would really be for the pull-request / merge way to go.
However, as I didn't really worked on/with the library for a while, I couldn't figure out what where the breaking changes in the API, could you give more precisions on this point ?
Some other small questions :

Scale - a vertical scale to go at the left edge of a plot (in contrast to the Grid Axis layer which is a possibly unbounded horizontal scale)

Do you think it would be possible to merge this in the Axis, or is it really a different approach ?

Piano roll - a sequencer note-type display (not currently editable)

Isn't the Segment layer enough to do that ? This example is made like that.

I will try to have more precise look on this in the next days, would be nice to collaborate on this.

@cannam
Copy link
Author

cannam commented May 19, 2017

Re. breaking changes, there are not many changes in the formal API, but every abstraction leaks, especially in Javascript! There are a few changes to the way things work that I fear might break real applications even if the API looks the same. For example, some existing shapes had to be modified to take into account the fact that the SVG now only covers the visible area (which changes the coordinate system for updating even though the same time-to-pixel mapping functions still work), so applications that add their own shape types might break, or we might have overlooked problems with shapes that we aren't using.

It might be revealing to try out our version of the library with some of your more complicated examples - I don't know how much work that would be.

You're quite right about the pianoroll - in fact for pianoroll we just added a small layer helper; it does use segment shapes and is much like the one in your example.

@b-ma
Copy link
Contributor

b-ma commented May 25, 2017

Hey,

Sorry for the late reply, I didn't have time to work on that and won't be available in the next weeks. I will probably work back on the library in July so, can we see all this more precisely at that moment or do you have some deadlines before ?

@cannam
Copy link
Author

cannam commented May 25, 2017

We have no particular time constraint - we can continue pulling in our own fork or switch to a merged version whenever.

@b-ma
Copy link
Contributor

b-ma commented Jul 5, 2017

Hi @cannam,

I finally had some time to check your changes more precisely, and also tried your fork on some more advanced demos we have made. So far, I'm still really for merging the two code bases as I really think the library can only benefit from that, however I have a few more questions / remarks:

  • the ability to resize a layer is broken (as can be seen in the advanced example of the waveform https://piper-audio.github.io/waves-ui-piper/examples/layer-waveform.html vs https://wavesjs.github.io/waves-ui/examples/layer-waveform.html), is this intentional or just a side effect of some other change ?
  • the accessor system on the data looks broken too (I tested with the segment), is this something intended too ? I really think its an important feature, event if I agree that it's maybe not the best solution in term of efficiency and should maybe be rethinked.
  • I have seen in the logs that you implemented a sonogram at some point, why did you remove it, performance problems ?

Also, one of the big changes I'd like to make as soon as possible is to remove the vertical shift of the y axis, this would potentially allow to embed canvas into the svg tree for complicated shapes like waveforms or sonogram without all the bugs it creates now, do you have any thought on this ?

Bests,

@cannam
Copy link
Author

cannam commented Jul 5, 2017

the ability to resize a layer is broken

The two waveform example links actually have different timeline states in the advanced versions -- one uses ContextEditionState, the other CenteredZoomState. I don't remember when we made this change but I guess it was initially a quick hack to test out CenteredZoomState because that was the mode we intended to use. I suspect ContextEditionState hasn't been tested at all since our changes.

the accessor system on the data looks broken

I'm not sure what you're referring to here (I mean I'm not even quite clear whether we're talking about a UI feature or a pattern in the code?) -- can you point to an example? The fact that I don't immediately know what this is, probably does make it likely that I've managed to break it!

I have seen in the logs that you implemented a sonogram at some point, why did you remove it, performance problems ?

We initially added a time-frequency spectrogram layer helper, which performed a series of short-time Fourier transforms and showed the result in a matrix shape. We removed the spectrogram layer because it didn't seem very appropriate to include all the FFT implementation, window shaping, etc into this library, especially since there are so many different ways to do a spectrogram in terms of window sizes, overlap, normalisation, etc. The matrix shape is still there (https://piper-audio.github.io/waves-ui-piper/examples/layer-matrix.html) and can still be used for such visualisations if you provide the time-frequency data. It works well enough to be used for files of pop-song length at least.

Also, one of the big changes I'd like to make as soon as possible is to remove the vertical shift of the y axis... do you have any thought on this ?

The detail of the vertical layout is not something I've looked at closely, we really just cargo-culted the Y axis stuff into our new shapes without studying the reasoning behind it. With regard to the canvas, I've been surprised by how well it's possible to do without it -- initially I thought it would end up being essential for complex shapes, but it is at least possible to get a pretty nice waveform and usable spectrogram without. That was a useful lesson for me from the waves-ui code, the conceptually tidy SVG-only output is not something I would have expected to work so well.

@b-ma
Copy link
Contributor

b-ma commented Jul 5, 2017

ability to resize a layer

ok, I will try to check what's wrong when I have some time, but I guess it comes from the changes you have made in the way the rendering context works. It's not something I really need for now, but would be nice to fix it at some point as it allows to have DAW-like interactions with audio files (kind of cropping).

accessor system

The "accessor" system is the way you tell the library how to access the data (e.g. this.x(datum)), I have checked your code more closely and it doesn't seem to have moved so it's probably something else, I will try with a more isolated test to see what's wrong.

spectrogram layer

Ok, I got the point, I agree all feature extraction stuff must be kept outside this library. In our "ecosystem" we use https://github.com/wavesjs/waves-lfo for this. Nice to have the matrix.

vertical shift

All layer are shifted vertically to have a kind of cartesian coordinate system instead of the default one with the y axis pointing to the bottom. This is something I have done to make reasoning about shapes and interactions easier but I'm retrospectively not sure if it's a good idea. The thing I'm sure however is that it creates really weird bugs when trying to embed a canvas into the svg (using the foreignObject tag, which would be a nice solution for shapes that are heavy to draw and inherently non interactive), but I have to admit I'm not sure that this would fix all the bugs neither...
I will try to do some tests and see what it implies.

What about having a skype in the next few days to see how to continue with that (email on my profile page if needed) ?

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

No branches or pull requests

2 participants