Need a new Category - for Bugs/Coding

[najak3d]

As collaboration begins to help complete the GraphicsControls, we should have a new category for these discussions.

Here's a starting point. I am reviewing some of the existing code, and found this inefficiency at the heart of it.

DrawMapper.DrawLayer (is called for each layer, for each control, for each Draw cycle -- so many times). This method calls:

 var action = Get(key)

which is a Dictionary Look-up to find the correct Draw Action to invoke.

Since for any given control type, the List-of-Layers is a fixed-order list, the Actions should also be contained in an Array. If the Order ever changes, then an event can fire to re-bulid this Action-List. So these Dictionary lookups should only be called One-Time for each control type, rather than being called without restraint as they are now.

When it comes to a Library such as "Graphics.Controls", it's vital that they are "efficient" so as to leave more CPU cycles for the applications using them. I'd be happy to write this fix for you now, and it can be my first collaboration.

[jsuarezruiz]

You're right. At the moment I have created 4 new labels: critical (mostly crashes or really blocker issues), high (something not working at all and has a high impact), medium (something does not work and is considerably important) and low (aesthetic details, format in code or errors at the sample level).

I think what you mention is important (high performance is one of the keys of this library) and I think it would be a good first contribution.

[najak3d]

Here's the basic concept that I just coded up for "DrawMapper"... the idea is to change the interface into something that simply returns an IEnumerable<> of LayerDrawCommands. So I've added "GetDrawActions()" to DrawMapper.

		private readonly LayerDrawAction[] s_NullList =	new LayerDrawAction[0];

		public virtual IEnumerable<LayerDrawAction> GetDrawActions(string[] layers)
		{
			if (layers == null)
				return s_NullList;

			LayerDrawAction[] layerDrawActions;

			if (!_drawActionCache.TryGetValue(layers, out layerDrawActions))
			{
				List<LayerDrawAction> actions = new List<LayerDrawAction>(layers.Length);
				foreach (var layer in layers)
				{
					var action = Get(layer);
					if (action != null)
					{
						LayerDrawAction entry = new LayerDrawAction() { LayerName = layer, DrawAction = action };
						actions.Add(entry);
					}
				}
				layerDrawActions = actions.ToArray();
				_drawActionCache[layers] = layerDrawActions;
			}
			return layerDrawActions;
		}

		private Dictionary<string[], LayerDrawAction[]>	_drawActionCache = new Dictionary<string[], LayerDrawAction[]>();

		public class LayerDrawAction
		{
			public string LayerName { get; init; }
			public Action<ICanvas, RectangleF, IViewDrawable, IView> DrawAction { get; init; }
		}

The for GraphicsControlHandler, the "Draw()" method code now looks like this:

		public virtual void Draw(ICanvas canvas, RectangleF dirtyRect)
		{
			if (VirtualView == null || _drawMapper == null)
				return;

			canvas.SaveState();

			if (_layerDrawActions == null)
				_layerDrawActions = _drawMapper.GetDrawActions(LayerDrawingOrder());

			foreach (var action in _layerDrawActions)
			{
				action.DrawAction.Invoke(canvas, dirtyRect, Drawable, VirtualView);
			}

			canvas.ResetState();
		}

		protected IEnumerable<DrawMapper.LayerDrawAction> _layerDrawActions;

Currently, if for some reason the Layers Order ever changes, you just need to set "_layerDrawActions = null" and it'll re-figure the list of DrawActions (once more).

A similar thing needs to be done for "MixedGraphicsControlHandlers"... Which means I'd need to refactor the "LayerDrawAction" concept to probably be a more generic ICommand interface of some sort. So you just call "Execute(args)" or something.

I'm out of time for the night, but just felt like dabbling.

[najak3d]

Looking more now at MixedGraphicsControlHandler concept, and I see the DrawBaseLayer() only can be called once.... so all we need to know is "when" to call DrawBaseLayer() -- first, 2nd, 3rd? And just call it in the right sequence, sequenced along with our own Graphics-based draw commands. I didn't look at this in more detail yet, but looks simpler than I originally thought.

I do like how Maui enables "Mixed" mode like this... that seems quite useful, potentially.

Looks like we have a very good foundation here. Now all we need is a commitment from Microsoft to put some more money into this, and ensure it gets done well and in timely fashion.

[RChrisCoble]

Question, why would you go through the process of new'ing a List<>, adding items, then causing another allocation for ToArray(), then throwing away the original List<>? Your goal should be to eliminate as many allocations as possible if you're going for the best perf.

Just new the array straight away into s_DrawActionCache based on the layer size and place the items directly into it.

[najak3d]

For simplicity sake. What you don't realize is that the "list of layer names" doesn't mean that all layers have actions -- some are Null (note the null-check inside that loop). And therefore the actual size of the array is unknown at the start. Since this operation is literally only run ONCE per control in your UI, the optimization you mentioned IMO, becomes negligible, as the List created will simply be collected in the next GC.

If this were a concern -- the next optimization I'd do, is to make the List<> a shared/static list, and then lock(List<>) before using it. That way, every Control Type that build's it's actions, just re-uses this same list, and we save the object creation and GC for each control.

But as I said, since these literally only run ONCE per Control Type -- which means it will probably only run < 100 times! Which is not worth making the code more complex for sake of that savings.

[RChrisCoble]

For simplicity sake. What you don't realize is that the "list of layer names" doesn't mean that all layers have actions -- some are Null (note the null-check inside that loop). And therefore the actual size of the array is unknown at the start. Since this operation is literally only run ONCE per control in your UI, the optimization you mentioned IMO, becomes negligible, as the List created will simply be collected in the next GC.

If this were a concern -- the next optimization I'd do, is to make the List<> a shared/static list, and then lock(List<>) before using it. That way, every Control Type that build's it's actions, just re-uses this same list, and we save the object creation and GC for each control.

But as I said, since these literally only run ONCE per Control Type -- which means it will probably only run < 100 times! Which is not worth making the code more complex for sake of that savings.

and we save the object creation and GC for each control.

Yes, please make this immutable and shared across all the controls with the initial checkin please. Our usages will potentially have thousands of controls instantiated at a single time.

[najak3d]

@RChrisCoble - note, it only constructs that List<> once per Control TYPE... not once per Control. All repeated controls of the same type, will simply share the SAME array of LayerDrawActions[]. Look at the code again -- all successive instances of the same type, will obtain this list from the Dictionary lookup.

I'm guessing you aren't using more than 100 different Control TYPES, right?

[najak3d]

Also a concern here is the Error/Warning/Info Logging that will be used with this library. For example, if you are running with the DEBUG version of this DLL, I would like it to spit out it's logic to a log file with 1000's of info's, alongside warnings/errors.

Thus code that does Null-Checking, instead of just returning, could Log.Warning("Draw() called with null Virtual view."). Or use Assert(VirtualView != null, "msg...") which only runs in DEBUG mode.

And to make the Release mode DLL run faster, these null-checks could be omitted entirely, as it would simply throw an exception. The methods that "call Draw on Children" would have a try/catch -- which then logs an Error (
Exception when trying to draw Control {control}: {exception}". Thus you don't need to insert EVERY possible null-check into the code... you can just allow the exception, caught at a slightly higher level, which tells you enough about what went wrong.

The current method, "hides the issue", which can be even more confusing to a developer -- "hey, why isn't control X showing?"

[najak3d]

Also, Null checks that we're concerned about should probably be done only during Initialization of the UI -- rather than being done for "every control, every render frame" for properties that really should never change. So if they are non-null after Init, then we are good to go, without null checking them.

[najak3d]

Can you add a new Category? I'd like to build momentum on these discussions, and inspire more energy/contributions from those listening. Right now the audience is small -- but those listening, probably, like us, really care about this library.

===
Here's another suggestion for the "GalleryPageCSharp.cs" sample file. Since those using C# composition technique are using this in lieu of XAML, the common style of this code is "succinct/terse" as possible.

Thus I would suggest that you make use of the existing C# technique called "Collection Initializer" syntax, which allows you to populate Children of a List with "Children = { item1, item2, item3 }" notation.

So for example, your method "CreateTimePicker()" would then look like this:

		IView CreateTimePicker()
		{
			return CreateContainer("TimePicker", new StackLayout
			{
				Margin = new Thickness(12, 6, 12, 24),
				Children =
				{
					new Label { FontSize = 9, TextColor = Colors.Gray, Text = "Default" },
					new TimePicker(),

					new Label { FontSize = 9, TextColor = Colors.Gray, Text = "Disabled" },
					new TimePicker { IsEnabled = false },

					new Label { FontSize = 9, TextColor = Colors.Gray, Text = "Custom" },
					new TimePicker { BackgroundColor = Colors.LightSkyBlue, TextColor = Colors.White }
				}
			});
		}

For easy reference, this code CURRENTLY looks like this:

        IView CreateTimePicker()
        {
            var layout = new StackLayout
            {
                Margin = new Thickness(12, 6, 12, 24)
            };

            layout.Children.Add(new Label { FontSize = 9, TextColor = Colors.Gray, Text = "Default" });
            layout.Children.Add(new TimePicker());

            layout.Children.Add(new Label { FontSize = 9, TextColor = Colors.Gray, Text = "Disabled" });
            layout.Children.Add(new TimePicker { IsEnabled = false });

            layout.Children.Add(new Label { FontSize = 9, TextColor = Colors.Gray, Text = "Custom" });
            layout.Children.Add(new TimePicker { BackgroundColor = Colors.LightSkyBlue, TextColor = Colors.White });

            return CreateContainer("TimePicker", layout);
        }

[najak3d]

More suggestions for code changes, to DrawMapper:

1. 'internal genericMap' should become 'protected _genericMap' (it's only used by class and derivations, right?)
2. The "Action<ICanvas, RectangleF, IViewDrawable, IView>" should be defined as DrawMapper delegate like this:

class DrawMapper
{
    public delegate void DrawAction(ICanvas canvas, RectangleF dirtyRect, IViewDrawable drawable, IView view);
}

And then you can replace all usages of "Action<ICanvas, RectangleF, IViewDrawable, IView>" with something less tedious: "DrawMapper.DrawAction" (intelli-sense fills it in for you).

[najak3d]

QUESTION: How do you plan to implement themes/skinning such as "Light" vs "Dark" colors?

This too should be part of the sample Gallery app.

[najak3d]

Another Question:
Is DrawableType enum the future? Or will this be replaced with something dynamic, so that devs can add their own DrawableType categories?

Also, have you envisioned if you are going to allow this overall Style to be changed run-time from within the app? (like a Hot-Reload, but doesn't require Visual Studio) Avalonia has this capability in their ControlCatalog, via a combo-box.