I think it is completely fine to have a discussion like this before a final version.

OK, I would consider this "review-ready" at this point 😆.

I've updated the topic post with some details and examples. As mentioned there, I won't get to complete docs before RC2, but I think the interface is stable at this point (pending any feedback), so could go into the RC.

And I plan to do a hefty cleanup of the commit history once the discussion concludes here.

I've resolved a number of the Conversations above, but left some open in case you'd like to respond, otherwise feel free to Resolve them if you're satisfied.

Thanks!

@sternsc
This PR can be considered as an extension of the previous one which added much more functionality than a modification of labelOffset. Because you noticed there was this breaking change, I wanted to make sure the old functionality was still replicable. But again, that's only a small part of the additions.

So just as a heads up, your code will still likely need to be updated. Here's an example that recreates some of the things you missed about the previous version (in particular labelOffset, and strings appended to tick labels):

// ~ Old Plotter replicated functionality: x labels at top, with strings appended
(
var gridHeight, freq = 22;
p = 101.collect({ |i| sin((i/1000)*2*pi* freq)}).plot;
gridHeight = p.plots[0].plotBounds.height;

p.setGridProperties(\x,
	\labelAnchor, \topLeft,
	\labelAlign, \left,
	\labelOffset, 8 @ gridHeight.neg,
	\labelAppendString, " xxx",
	\constrainLabelExtents, true,
).setGridProperties(\y,
	\labelAnchor, \bottomLeft,
	\labelAlign, \left,
	\labelOffset, 3 @ -3
);
)

If you plan for your views to dynamically resize, you can make sure extreme tick label offsets stay in place this way:

(
// Because use the view height to set the x tick label offset, update label offset on resize.
p.interactionView.onResize_({ |v|
	var gridHeight = p.plots[0].plotBounds.height;
	p.setGridProperties(\x, \labelOffset, 8 @ (gridHeight.neg+10))
});
)

I've cleaned up the commit history.

Thanks @telephon and @dyfer for your helpful reviews!

Thank you @mtmccrea for your work on this and @telephon for the review.
@sternsc could you confirm that the changes here fix the issues you've encountered?

Unfortunately I won’t have time soon to try the new code – I don’t have a build environment for all of SC set up and ready to go.

For macOS and Windows you can download the builds from GitHub actions (see "details" next to the CI checks, or the "Checks" tab on top, then click "Summary" and find artifacts on the bottom right, after scrolling down).

Would it work to get only the relevant .sc files (Plotter, GridLines etc) from the latest version, and replace the ones in 3.12.2, just to try? If so, which files exactly?

If might. I'd try all the .sc files changed in this PR (see "Files changed" on top) and additionally the ones which were changed in #5161 (but of course taken from the branch in this PR).

the first thing you can try to just copy the whole class library over. If there weren't any changes to kernel classes and primitives, it might just work. If it doesn't, you could try copy the GUI classes.

the first thing you can try to just copy the whole class library over.

I didn't suggest this since AFAIR there were some changes to the interpreter and when I tried using class library from a recent develop on the SC 3.12 build it didn't work. But yes, this would typically be the first thing to try.

@sternsc here is code that reproduces the grids you showed in your previous post.

```
( // Example 1
	var win = Window("DrawGrid test",
		Size(350, 300).asRect.center_(Window.screenBounds.center)
	).front;
	var uview = UserView(win, win.bounds.size.asRect);
	var xspec = [30, 95].asSpec;  // MIDI
	var yspec = [45, 120].asSpec; // dB
	var inset = 2; // border around the grid
	
	d = DrawGrid(uview.bounds.insetBy(inset),
		xspec.grid, yspec.grid
	);
	
	uview.drawFunc_({ |uv| d.draw; })
	.onResize_({ |uv|
		d.bounds = uv.bounds
		.insetBy(inset)
		.moveTo(inset, inset);
	})
	.resize_(5)
	.background_(Color.white)
	;
	
	d.x
	.labelAnchor_(\bottomLeft)
	.labelAlign_(\left)
	.labelOffset_(3 @ -3)
	.labelAppendString_(" MIDI")
	.gridColor_(Color.green)
	.fontColor_(Color.green)
	.font_(Font(Font.defaultSansFace, 11))
	;
	
	d.y
	.labelAnchor_(\topLeft)
	.labelAlign_(\left)
	.labelOffset_(3 @ 3)
	.labelAppendString_(" dB")
	.gridColor_(Color.green)
	.fontColor_(Color.green)
	.font_(Font(Font.defaultSansFace, 11))
	;
)

( // Example 2
	var win = Window("DrawGrid test",
		Size(350, 200).asRect.center_(Window.screenBounds.center)
	).front;
	var uview = UserView(win, win.bounds.size.asRect);
	var xspec = [-4.8, 0].asSpec; // s
	var yspec = [0, 28].asSpec;
	var inset = 2; // border aroudn the grid
	var xtickoffset = 10;
	d = DrawGrid(uview.bounds.insetBy(inset),
		xspec.grid, yspec.grid
	);
	
	uview.drawFunc_({ |uv| d.draw; })
	.onResize_({ |uv|
		d.bounds = uv.bounds
		.insetBy(inset)
		.moveTo(inset, inset);
		d.x.labelOffset_(0@d.bounds.height.neg + xtickoffset)
	})
	.resize_(5)
	.background_(Color.black)
	;
	
	d.x
	.labelAnchor_(\topLeft)
	.labelAlign_(\left)
	.labelOffset_(0@d.bounds.height.neg + xtickoffset)
	.labelAppendString_(" s")
	.gridColor_(Color.gray)
	.fontColor_(Color.gray)
	.font_(Font(Font.defaultSansFace, 11))
	;
	
	d.y
	.labelAnchor_(\topLeft)
	.labelAlign_(\left)
	.labelOffset_(3 @ 3)
	.gridColor_(Color.gray)
	.fontColor_(Color.gray)
	.constrainLabelExtents_(false)
	.font_(Font(Font.defaultSansFace, 11))
	.numTicks_(5)
	;
)
```

Which produces:

(Including behavior for update on resize).

So I think the functionality you've described is covered here.

Thanks for your kind example @mtmccrea , it looks great! Looking forward to it!

Thanks so much @mtmccrea !
Sorry for looking into the details late in the conversation here.
Do I read correctly that in the process we lost automatic display of ControlSpec units? If so, I'd consider that as a regression from SC 3.12, where units were always displayed.
IMO they should still be displayed by default, but the question is whether that default should be next to the value, or along the axis.
I think it would be best to have a method to switch between these two options easily, e.g.;

DrawGridX/Y -unitDisplay(arg) where arg is \tick (appended to tick labels, 3.12 behavior), \axis (on axis label, "new" behavior), nil (not displayed). Either \tick or \axis` could be the default, depending what we all feel like.

What do you think?

Sorry for dragging this out!

Yes spec unit auto label functionality was removed (IIRC, in response to the last PR).

Some background:

`3.12.2` default behavior:

Just plotting a stereo buffer:

(
p = (4 ** (-5..0)).plot;
p.specs = \delay;
p.domainSpecs = [0, 10, \lin, 0, 0, " xxx"].asSpec; p.refresh;
)

`3.13-rc` default behavior:

`3.13-rc` to achieve the old behavior (with new space padding):

// ... after plotting the buffer
p.axisLabelX = nil;
p.setGridProperties(\x, \labelAppendString, " " ++ p.domainSpecs.first.units)

(
p = (4 ** (-5..0)).plot;
p.specs = \delay;
p.domainSpecs = [0, 10, \lin, 0, 0, " xxx"].asSpec; p.refresh;

p.setGridProperties(\y, \labelAppendString, p.specs.first.units);
p.setGridProperties(\x, \labelAppendString, p.domainSpecs.first.units);
)

Notice Buffer:-plot still shows units by default, to align with previous behavior, but shown in the axis label.
Function:-plot does as well, because in both cases the units can be inferred confidently, unlike, e.g., Array:-plot.
I opted to put the units in the axis label because I thought having units on every tick label was redundant and crowded, and axis labels can replace that functionality. The extra vertical space taken is a consideration, so it will omit the unit label when the channel count is >= 4.

Moving forward:
So you can still achieve the same behavior in 3.13. Now the questions are

• whether spec units should be shown by default (in all cases where a spec has units, rather than just Buffer and Function plots)
• if so, whether the units should display with tick labels or as the axis label

If I'm not mistaken, @telephon authored the original approach, so may have thoughts on this.

Moving forward: So you can still achieve the same behavior in 3.13. Now the questions are

Yes, but it requires extra code.

• whether spec units should be shown by default (in all cases where a spec has units, rather than just Buffer and Function plots)

I do think it should.

• if so, whether the units should display with tick labels or as the axis label

With this one I could go either way. I like the visual aspect of the "new" way, but it does take a bit more space around the plot.

I'm fine either way, there is much improvement.

OK thanks, I've made the units show by default (in the axis label—easy to change this default if we hear feedback from users in the RC).

I've also added these methods to the docs.

good! let us know when it is ready for merge.

It's ready on my end :)

Thank you @mtmccrea and @telephon !

Super, thanks for 3.13.0-rc2. I've tested the new Grid-related stuff and found the following:

1. in ExponentialGridLines.getParams there is a small bug (Grid.sc line 751 and following:
   ) { drawLabel = false // drop labels for tightly spaced upper area of the decade };
   But drawLabel must be made true again when increasing to the next decade:
   ) { drawLabel = false // drop labels for tightly spaced upper area of the decade } { drawLabel = true } ; // sternsc: bug fix: restore labels for the next decade
2. The warp property of a ControlSpec is set using a symbol (\lin or \exp), but the getter function now returns instead an object instance of a subclass of AbstractGridLines. This seems asymmetric and makes it more awkward to inquire for the type of warp. I suggests a getter method called .warper that returns the object, and keeping .warp to return the symbol. Maybe.
3. It would be great if .formatLabel could have a drawUnits argument in addition to the drawLabel argument. The reason is that it helps to plot the unit only on whole decades, as shown in this figure.

I've been able to work my way around to this result by overriding .getParams and twiddling with calls to .appendLabel, as shown by this code excerpt from the end of ExponentialGridLines.getParams:
`

		// p['labels'] = lines.collect({ arg arr, inc;
		// 	var val, drawLabel, thisLabel;
		// 	#val, drawLabel = arr;
		// 	[val, this.formatLabel(val, nfrac ? nfracarr[inc] ? 1), nil, nil, drawLabel.not];
		// });

		//// sternsc's exponential version: draw the label units only at whole decades
		//// It could be neater if .formatLabel had also a drawUnits argument
		p['labels'] = lines.collect({ arg arr, inc;
			var val, drawLabel, thisLabel;
			var saveUnits, retVal;
			#val, drawLabel = arr;
			saveUnits = this.appendLabel;
			if (val.abs.log10.frac > 0.01, { this.appendLabel_(nil) });
			retVal = [val, this.formatLabel(val, nfrac ? nfracarr[inc] ? 1), nil, nil, drawLabel.not];
			this.appendLabel_(saveUnits);
			retVal
		});
	};
	^p

`
4. The usage of the numTicks argument with exponential axes needs to be clarified. So far as I can tell, it means approximately the number of ticks per decade: 1 or 3 or 10; where three gives 1,2,5 (fine!) and anything more than 4 gives up to 10. The default behavior is a bit clunky, so I found myself changing numTicks in the view's .onResize method. This probably can't be automated because it depends on font sizes and units strings as well.

All the linear stuff works fine and I have gotten the grids plotted as I like them. Thanks a lot for your work on this!

Hope this is of some help. And I can live with overriding - you can't cater to everyone's preferences...