Efficient code question

hey,
i’d like to know if theres a more efficient way to do what i’m currently doing. Not that it’s causing problems, its actually very fast and responsive, but the more experienced devs can maybe point out stuff i shouldn’t be doing etc.

My example app:

I want to build a piano roll, and this is the side project i created to test grid drawing. right now the “notes” are only 16*16 squares but will be turned into seperate components that can be dragged and scaled.

heres the code to find the grid box to draw into and the function to create the note:

        void mouseDown(const MouseEvent& e) override
		{
			Component::mouseDown(e);
			auto x = getBeatPositionFromX(getMouseXYRelative().getX());
			auto y = getNoteFromY(getMouseXYRelative().getY());
			drawNewNoteAt(x, y * note_height);
			repaint();
		}

        int getNoteFromY(float y)
		{
			return (int)jmap(y, 0.f, (float)getHeight(), 0.f, 144.f);
		}

		int getBeatPositionFromX(float x)
		{
			return (int)jmap(x, 0.f, (float)getWidth(), 0.f, 64.f);
		}

        Array<Rectangle<int>> notes_in_sequence{};
		void drawNewNoteAt(float x, float y)
		{
			notes_in_sequence.add(Rectangle<int>{ (int)x * 16, getNoteFromY(y) * 16, 16, 16 });
		}

and heres the paint call for the grid and notes:

        void paint(Graphics& g) override
		{
			g.fillAll(Colours::black);

			g.setColour(Colours::white.withMultipliedAlpha(0.2f));

			for (int n = 0; n < getHeight(); n++)
				if (n % 16 == 0)
					g.drawHorizontalLine(n, 0.f, (float)getWidth());

			for (int n = 0; n < getWidth(); n++)
				if (n % 16 == 0) 
					g.drawVerticalLine(n, 0.f, (float)getHeight());

			g.setColour(Colours::indianred);
			for (auto& note : notes_in_sequence)
				g.fillRect(note);

		}

very much appreciate your helpful information!

Actually, this looks good to me. You could use e.x and e.y instead of getMouseXYRelative(). The mouse Event (e) holds a lot of cool features that you can use.

I wouldn’t use components for notes. In the mouseDown function I would insert the note in to the midi sequence. In the paint function I would iterate over the midi sequence for getting the positions of the single notes and paint the notes.

You could also cache the grid into an image to avoid to draw it each time.
Don’t ask me how, i havn’t reached that part of JUCE for now! :laughing:

You could reduce the iterations here, and remove the need for the condition by just incrementing by 16 each time:

for (int n = 0; n < getHeight(); n += 16)
    g.drawHorizontalLine(n, 0.f, getWidth());
4 Likes

As @baramgb said, do not use components for notes. It’s a big red flag. If you do so, although the code may look cleaner, you’ll get a huge performance hit when the user creates lots of notes.

Simply do what you’re already doing and play with hit testing to edit notes.

2 Likes

You could maybe have a component for each row to avoid needing to dynamically create potentially thousands of components (assuming you have a fixed number of rows).

Using separate components would improve the graphics rendering time as you wouldn’t need to re-draw the entire window every time a note is changed, only the relevant components and maybe the background. With simple graphics like shown the difference will be negligible but if the graphics get any more complex it would be a noticable difference.

Since notes are mostly only Rectangles it is not very performance impacting to draw. I think that updating 127 components would be much more complicated than 1 component with a lot of rects.

If everything was one component, then every time you add a note and so need to repaint, you’d have to repaint literally everything - drawing a lot of pixels, and potentially having to loop over thousands of notes across the whole grid.

If you have one component per row, then when you add a note to that row you only need to repaint that single component as only it’s bounds will become invalided. So in theory you only need to redraw 1/128th of your window for each note you add or remove.

1 Like

That is not true. You can pass bounds to repaint in order to repaint only a specific area.

That being said one component per row is reasonable.

1 Like

But the whole paint method will still be called, in which you’ll probably be iterating all your notes, and all the lines. There’s nothing you can do to reduce the call time of a paint method without simplifying it. Just because you only then render a portion of that UI to the screen, doesn’t mean somehow only part of your paint method is called.

Well it’s quite easy to read the clip bounds in paint and perform a binary search on notes to find the relevant ones.

Of course for this to work the notes need to be sorted by pitch and time, which is also trivial.

That sounds like more effort than just using separate components IMO - but that does sound like a reasonable solution.

Could be. These are all reasonable solutions, it’s all about trade-offs :slightly_smiling_face:

The efficiency is not about to use component or not. If you use components it makes sense to only have the ones visible and have a mechanism to recycle the components similar to the ListBox with the CustomComponents.
If you iterate over a huge area of notes where many of them may be invisible, it makes sense to have the notes sorted and do a binary search for the first visible note and the last visible note to avoid traversing the whole vector/array.
However, if it is just 100-500 notes (like for an rhythm pattern etc) simply iterating and continue; when it is outside might be faster than the other approach.

Using Components has the advantage that the implementation of mouse interaction is much nicer and maintainable.

But that question has as many answers as developers around.

And just to add my two cents, as someone who has developed a DAW, drawing the midi notes as rectangles is very fast. A whole screen full of midi notes from many midi tracks paints instantaneously. All you do is iterate over the notes;

for (auto note : midiClip->getSequence().getNotes())
{
	const auto noteBounds{ getNoteBounds(note) };
	if (localBounds.contains(noteBounds))
	{
		g.setColour(Colours::darkgrey);
		g.fillRect(noteBounds);
	}
}

It paints in a flash and that includes painting the piano roll background (for which I did not show the code).

Try it. Computers do this sort of thing very fast.

4 Likes

That looks a bit inefficient to me :slight_smile: . I would rather do something like

	RectangleList<float> rectList;

	for (auto note : midiClip->getSequence().getNotes())
	{
		const auto noteBounds{ getNoteBounds(note) };
		if (localBounds.contains(noteBounds))
			rectList.addWithoutMerging(noteBounds);
	}

	g.setColour(Colours::darkgrey);
	g.fillRectList(rectList);

4 Likes

Sure, that is another way to do it. I was simply pointing out that it is a very fast operation.