Add a new "Event Marker" path type

[MatthewScott967]

Hello Leon,

I am involved in a project that is using Ball Aerospace's Cosmos project to visualize telemetry. That project used uPlot as the basis for the Vue based graphs widgets.

One of the things that I need to identify in my telemetry streams are "events". These are interesting events that happen in the same timeline as the rest of the telemetry that is being reported. Rendering these "Event Marker" are a critical part of interpreting the data being graphed.

Here is a demo that I wrote that visualized my changes to uPlot and the functionality I need.

I went a head and tried to do this work in a way that seemed constant with what you have done with uPlot with the idea being that if you are interested in merging my work, it will just make it that much easier for me to take advantage of it in my project.

I am quite happy with what I have come up with, and will be using it in my project, but thought it could be pretty useful for others.

As far as details of my change, I really just added a new path type. However, I needed to extend the return from the path function to include a "labels" array. This is critical for me to draw the string label, but it seemed like a reasonable extension, so other path functions could place labels on the path.

It is the case that my work breaks your rule that the "Y values much be numeric". For my usage, that is not an issue, but I could totally understand if you don't want to consider this work, due to this violation.

The only thing I got hung up on, because my expertise is not in Javascript programming, is how to "know the type of the path function" on line 899 of uPlot.js. I want to make sure I set series.auto to false, because trying to use the string to scale the axis screws things up. What I came up with works, but not very happy with that solution.

Thanks for taking the time to consider merging my work. Clearly not a problem is you think it is not appropriate. However, if you are interested, that would be great. If so, and you want me to change things around, let me know and I am happy to address any feedback / comments you have.

Cheers,
Matt

[leeoniya]

Thanks for the PR, Matt :)

I think this is a great thing to have as an external plugin & demo, however I'm pretty conservative when adding features to the core, but can certainly be convinced otherwise when there is a strong case for it. Let me explain.

Generally, I've tried to avoid core features that render text on the canvas, except tick values, which is of course unavoidable. Part of the reason for this is that people generally expect text not to collide, so any core feature that offers text rendering must also solve the collision and layout or skipping problem. On a continuous axis where intermediate values can be inferred this is manageable, but when you have things like event labels, it becomes unclear what can/should be skipped, etc. In this situation, we must now offer not only some kind of algorithm, but also a bunch of options that can be tweaked to satisfy 99% of use cases. Next, people often want annotation labels to be clickable and/or tooltip-hoverable, which means we need to measure and keep track of bounding boxes and a quadtree plus track cursor interaction...or render them in HTML instead of canvas (probably much easier). Additionally, point and range annotations are frequently mis-aligned with raw data timestamps, labels rendered inside plot area can obscure data. These are just some considerations, but there are many others that will remain unaddressed in a simple implementation, thus disappointing many devs, and causing them to write their own anyways.

I've added a basic HTML-based annotations demo [1] that is interactive (hover changes style via CSS), and can be extended much further to support tooltips with extra info, click-to-go, etc. The annotations are not rendered as series, so lack toggling via legend and can be in arbitrary x locations and do not have to fit into the alignedData object. I have a near-term goal of figuring out a way to pass data through alongside AlignedData so that it can be used from within plugins like this one -- currently you have to work around it a bit with closures/getters because u.data only holds AlignedData.

I'm curious what you think of this strategy and whether there is something that is possible with an in-core addition that cannot be done externally.

[1] https://leeoniya.github.io/uPlot/demos/annotations.html

[MatthewScott967]

Thanks for the feedback, and honestly, it was basically what I was expecting. You are (or course) 100% correct that the simple text layout of the path labels that I did is insufficient for arbitrary usage patterns. It is something that could easily be added since it is already the case that styling can be applied to the paths and path points, although it is true there would need to be additional types added for full text label support.

I may look again at doing something like the annotations demo, but in my case, the graph values are being streamed and the higher level code (Ball Aerospace Cosmos) is already dealing with providing data from disjoint sources to the same graph, in the format uPlot requires. Therefore, the simple & clean "events as just another series" is really the ideal pattern for my use case.

So, I totally understand and agree with your feedback, please go ahead a close the PR. If I choose to do a more plugin based approach I will let you know and potentially submit that as a demo / plugin. However, it is likely I will just use my own fork with these changes.

I appreciate your time and feedback.

Cheers

[leeoniya]

no worries, i think my main point is that this should be doable without modifying the core. it would be cool to figure out exactly what minimum additions the core would need to support your exact case -- i think it can already work without any modifications.

most of the stuff you've put into the core can be pulled out into the demo itself.

you can definitely store text labels in the AlignedData object as long as you set series.auto: false to prevent uplot from attempting to auto-range from text. then just provide your eventMarkers() builder for those series. you can stick additional label config onto each series' config and then use a drawSeries hook to render the label text on top of the pathBuilder paths (and can still read/re-use the config and data off the uplot instance).

there are plenty of demos that take this approach and work well.

one part that could be improved is having to set up a drawSeries hook and re-compute the boundaries, but this is pretty minor. at some point in the future pathBuilders will be able to return an array of layers and rendering fns. something like:

return [
  {
    stroke: Path2D,
    fill: Path2D,
    ...,
    style: {
      fill: "red",
      stroke: "green"
    }
  },
  {
    stroke: Path2D,
    fill: Path2D,
    ...,
    style: {
      fill: "red",
      stroke: "green"
    }
  }
  myDrawTextOnCtxFn,
];

[leeoniya]

i wonder if simply adding the capability of {stroke: Path2D, fill: Path2D, text: myDrawTextOnCtxFn, ...} to the pathBuilder return signature would help. this way the core can execute this returned text fn after the paths are drawn and it can reuse any bounding boxes or text content computed inside the pathBuilder closure.

one question with this approach is whether we want to also add an option to allow text to be drawn after all path have been rendered to avoid situations where later paths can occlude text drawn with earlier paths.

[MatthewScott967]

Ah OK, I get it now, thanks for the additional bread crumbs here. Let me take a look at making revisions this weekend, but I do now understand what you are getting at!

Cheers

[MatthewScott967]

Wow, again, thank you for leading me through how to do this all external to the core code. I have followed your suggestions and all most all code is now external to the core.

I liked your idea of having a text label drawing fn as part of pathBuilder return signature so I put that in. It seems really nice and clean. If you prefer I implement this as a hook, I am happy to change to that.

The only thing I don't like is that using eventMarkers.js, you must set series.auto and a series.value. every time you define a series. Not the end of the world, but it can bite you. As you point out, if you don't set series.auto, then the entire graph breaks (bad scale).

Not sure if you have any hints for me to look at to address that problem.

Let me know if you would consider merging this (now) demo code. If so, is there anything else you would like me to clean up? Including removing the text fn to drawSeries.

Thanks again for actually helping out and getting me to understand your initial comments, this is clearly a much cleaner way to go.

[tve]

I have not (yet) looked into the code details, but am highly interested in this feature, whether in the core or as external add-on. Thanks for posting!