VIX-3697 New Arch Prop by da-markson · Pull Request #738 · VixenLights/Vixen

da-markson · 2025-10-17T04:13:20Z

The code needs a complete review as this will be the basis for future props.

Complete working version of the Arch prop wizard and Setup Display.
Added the Dimming Curve into the Wizard
Incorporated the new OpenGL preview

Other notes:

The Tree prop is now broken. I will fix all the issues once the Arch prop coding and processes are reviewed and approved
The Display Setup of the Arch prop is still using simple text boxes and the like. The custom implementation of the WPF grid is proving challenging to modify to support implementing more user friendly UI components. I'll work on this next.
The CategoryOrder attribute is not working, so the current display ordering of the categories doesn't make sense. This too may be because of the customization of the WPF grid that I'll explore. The intended ordering should be Attributes, Rotation, Dimming Curve, Creation.
I also need to add in Color handling into the Wizard. Once everything else is reviewed, I'll create a wizard page for that too.

johncbaur

Probably should address high level comments in confluence before tackling the comments identified here.
https://vixenlights.atlassian.net/wiki/x/AwALDQ

src/Vixen.Application/SetupDisplay/ViewModels/PropNodeTreeViewModel.cs

src/Vixen.Modules/Editor/FixtureWizard/FixtureWizard_sreonfzn_wpftmp.csproj

src/Vixen.Modules/Editor/PropWizard/PropWizardBase.cs

src/Vixen.Core/Sys/Props/Components/PropComponent.cs

src/Vixen.Core/Sys/Props/Model/BaseLightModel.cs

src/Vixen.Modules/App/Props/Models/AxisRotationViewModel.cs

src/Vixen.Modules/App/Props/Models/Arch/ArchModel.cs

src/Vixen.Application/SetupDisplay/Wizards/Pages/DimmingWizardPage.cs

src/Vixen.Application/SetupDisplay/Wizards/Views/DimmingWizardPageView.xaml

…g major changes

jeffu231

I am not sure if this is still WIP, but many of the comments have yet to be addressed with these last updates. The Extended.Wpf package needs to also have a replacement since its license prevents us from using it. If it is still WIP, then I will await further comment until you are ready.

da-markson · 2026-02-16T16:39:19Z

Absolutely WIP. I've made A LOT of changes over the last few weeks and am conducting testing now. I hope to have something to evaluate for everyone on Wednesday. I will write all the changes up in Confluence. Thanks, David Markson *Cell*: (817) 584-4755 *E-mail*: ***@***.***

…

On Mon, Feb 16, 2026 at 10:31 AM Jeff ***@***.***> wrote: ***@***.**** commented on this pull request. I am not sure if this is still WIP, but many of the comments have yet to be addressed with these last updates. The Extended.Wpf package needs to also have a replacement since its license prevents us from using it. If it is still WIP, then I will await further comment until you are ready. — Reply to this email directly, view it on GitHub <#738 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BMJ3GZRAE6CN5IDAVRYQSD34MHWFTAVCNFSM6AAAAACJN67Y7WVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTQMBZGU2DCOJVGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

da-markson · 2026-02-17T02:36:08Z

A lot of changes. Please review the Confluence page for details

johncbaur · 2026-02-20T12:33:26Z

src/Vixen.Core/Sys/Props/HelperTools/PropParameters.cs

@@ -0,0 +1,58 @@
+namespace Vixen.Sys.Props.HelperTools
+{
+	public class PropParameters : Dictionary<string, object>


Still trying to figure out the purpose for this helper class but if we continue forward with it, I think all usages of it should use the nameof() method on properties instead of string constants. This should allow developers to rename properties and this code will go along for the ride.

UPDATE: So I have reviewed more of the changes. I don't think we can use the nameof because the explicit properties have been removed and been replaced with the Dictionary. I think this PropParameters has some downsides:

Using strings as keys to this Dictionary is inefficient when compared to directory property usage.

It also makes it difficult to rename properties as we will have to search for the strings.

I think it might be harder to read and inspect the code.

Prevents using reflection to discover properties

May complicate XML serialization if we go down that path

Prevents being able to implement INotifyPropertyChange

Prevents putting additional code in the setter of the property

johncbaur · 2026-02-20T12:36:12Z

src/Vixen.Core/Sys/Props/BaseProp.cs

+				}
+				else if (!VixenSystem.Props.IsUniquePropTitle(value))
+				{
+					MessageBox.Show($"{value} already exists. Enter a unique name.", "Prop name must be unique", MessageBoxButtons.OK, MessageBoxIcon.Error);


My objection is with the MessageBox. Models should not directly spawn UI's. I think we should strive for clear separation of business rules and UI.
Not sure it reaching out to the database of props is a good idea.
This is just speculation but what if I created a temporary Prop for some use case and as soon as the name is set it pops up this message box when the prop was going to get destroyed?

johncbaur · 2026-02-20T12:40:51Z

src/Vixen.Modules/App/Props/Models/BaseLightProp.cs

-
+
+		private ObservableCollection<AxisRotationModel> _rotations;
+		public ObservableCollection<AxisRotationModel> Rotations


Rotations should not be unique to a LightProp.cs. I think this property was originally on the model at the base level. I sort of would like to do a deeper dive discussion with @jeffu231 on why we need both a model and a prop. Seems like if we combined these classes we might make our lives easier. Need to have a discussion before taking any action on this comment.

johncbaur · 2026-02-21T02:04:32Z

src/Vixen.Modules/App/Props/Models/AxisRotationViewModel.cs

Need to move this class back to Vixen.Application ViewModels folder.

johncbaur · 2026-02-21T02:22:29Z

src/Vixen.Application/SetupDisplay/ViewModels/PropNodeTreeViewModel.cs

+		public static readonly IPropertyData IsSubNodeProperty = RegisterProperty<bool>(nameof(IsSubNode), false);
+		#endregion
+
+		private void OnIsTopNodeChanged()


This method appears not be used.

johncbaur · 2026-02-21T02:26:57Z

src/Vixen.Application/SetupDisplay/ViewModels/PropNodeTreeViewModel.cs

+		}
+		public static readonly IPropertyData IsTopNodeProperty = RegisterProperty<bool>(nameof(IsTopNode), true);
+
+		public bool IsSubNode


If IsTopNode and IsSubNode are inverses of each other used for visibility there are several methods of handling that in the XAML with InverseBooleanToVisibilityConverter. Only change if this appeals to you.

johncbaur · 2026-02-21T02:28:12Z

src/Vixen.Application/SetupDisplay/ViewModels/PropNodeTreeViewModel.cs


 		#endregion

+		#region Temporary properties to get Change Prop to work


This is a strange region name. What is meant by Temporary?

johncbaur · 2026-02-21T02:35:05Z

src/Vixen.Application/SetupDisplay/ViewModels/PropNodeTreeViewModel.cs

+			IPropFactory newPropFactory = PropFactory.CreateInstance(propType);
+
+			// Create a default Prop
+			(IProp newProp, IPropGroup propGroup) = newPropFactory.CreateBaseProp();


This is where I noticed that supporting groups got broken. The previous design was that the prop(s) were not created until the user had gone through Wizard to know how many props to create. Seems like to me we may need to hide the Group page when editing the prop after initial creation. We also probably need to look into allowing the user to just hit finish rather than forcing them to go through all the pages. I think Nick was asking for this feature as well.

johncbaur · 2026-02-21T02:55:18Z

src/Vixen.Application/SetupDisplay/ViewModels/SetupDisplayViewModel.cs

+		internal void UpdatePreviewModel(IProp prop, bool force = false)
 		{
+			if (force == true)
+				DrawProp(prop?.PropModel);


I don't think we want to have this force argument. Is there a strong need for it? If we continue to support a prop preview and a full setup preview, we have two OpenTK contexts running simultaneously. My testing showed that this could result in only one of the two actually drawing because the other one seemed to get confused.

I tried to document this on the DrawProp method.
///
/// This method needs to be called within the OpenTK control render method.
/// Otherwise the two OpenTK controls get confused and the prop preview won't render properly or consistently.
///

johncbaur · 2026-02-21T02:56:52Z

src/Vixen.Application/SetupDisplay/ViewModels/SetupDisplayViewModel.cs

+
+        #endregion]		
+
+        private Axis GetAxis(string axis)


This method appears to be no longer used.

johncbaur · 2026-02-21T02:57:06Z

src/Vixen.Application/SetupDisplay/ViewModels/SetupDisplayViewModel.cs

+	        };
+        }
+
+        private string GetAxis(Axis axis)


This method appears to be no longer used.

johncbaur · 2026-02-21T13:40:12Z

src/Vixen.Application/SetupDisplay/Views/PropNodeTree.xaml

                <ResourceDictionary Source="../Themes/Theme.xaml"/>
            </ResourceDictionary.MergedDictionaries>
-
+            <BooleanToVisibilityConverter x:Key="BoolToVis"/>


This XAML file already has a BooleanToVisibilityConverter.

johncbaur · 2026-02-21T13:45:50Z

src/Vixen.Application/SetupDisplay/Wizards/Factory/PropWizardFactory.cs


 namespace VixenApplication.SetupDisplay.Wizards.Factory
 {
+	public static class PropFactory


Could this just be another method in the PropWizardFactory?
If not, best practice is that each class gets its own cs file.

johncbaur · 2026-02-21T14:03:10Z

src/Vixen.Application/SetupDisplay/Wizards/Pages/ArchPropWizardPage.cs

-			NodeCount = 25;
+			Title = "Basic Attributes";
+			Description = $"Enter attributes for {PropType.Arch.GetEnumDescription()}";
+			NodeCountMinimum = 3;


Do we want a NodeCountMaximum? I know all my arches are ~70 pixels, so 100 is not very large. You see videos of putting arches over sidewalks and driveways. I would imagine a driveway would be more than 100 pixels. Seems like the maximum could grow to something unreasonable like the size of short or int.
The legacy Arch smart object seems to let the count grow.

johncbaur · 2026-02-21T14:19:17Z

src/Vixen.Application/SetupDisplay/Wizards/Pages/ArchPropWizardPage.cs

-		/// Gets or sets the NodeCount value.
+		/// Gets or sets the number of lights.
 		/// </summary>
+		/// <remarks>


I believe the Wizard pages are effectively models. We shouldn't need to create Catel properties on the models.

johncbaur · 2026-02-21T14:24:34Z

src/Vixen.Application/SetupDisplay/Wizards/Pages/ArchPropAdditionalWizardPage.cs

+		/// <summary>
+		/// Gets or sets if the wizard will additionally generate a left and right prop.
+		/// </summary>
+		public bool LeftRight


The wizard pages are the model and I don't believe need to be Catel properties.

johncbaur · 2026-02-21T15:38:34Z

src/Vixen.Core/Sys/Props/Model/BaseLightModel.cs

 		/// </summary>
 		/// <param name="nodePoints"></param>
 		/// <param name="angleInDegrees"></param>
 		protected static void RotateNodePoints(List<NodePoint> nodePoints, int angleInDegrees)


This method is only used by the Line model which really isn't in use anymore.
We should clean this up.

johncbaur · 2026-02-21T15:55:35Z

src/Vixen.Modules/App/Props/Models/Tree/TreeModel.cs

 			// (Optionally) rotate the points along the X, Y, and Z axis
-			RotatePoints(treePoints);	
+			//ToDo : Replace null with rotation
+			RotatePoints(treePoints, null);	


Rotations on the tree were working. Not sure why this code was changed to pass null.
Not sure why the argument was added.

johncbaur · 2026-02-21T16:01:07Z

src/Vixen.Modules/App/Props/Models/Tree/TreeModel.cs

-		private IEnumerable<NodePoint> Get3DNodePoints(float width, float height)
+		protected override IEnumerable<NodePoint> Get3DNodePoints()
 		{
+			float width = (float)PropParameters["Width"];


I am not sure the prop models need a width and height.
The props were rendering with a normalized with and height of 1x1.

Suggest setting width and height local variables to 1.0f here.

johncbaur · 2026-02-21T16:07:01Z

src/Vixen.Modules/App/Props/Models/BaseLightProp.cs

 		protected BaseLightProp(string name, PropType propType) : base(name, propType)
 		{
 			StringType = StringTypes.Pixel;
+			Rotations = new ObservableCollection<AxisRotationModel>();


This is probably a duplicate comment but Rotations belong in BaseProp.

johncbaur · 2026-02-21T16:11:43Z

src/Vixen.Core/Annotations.cs

  /// </summary>
  /// <syntax>
-  /// <p>Function Definition Table syntax:</p>
+  /// <p>Function Definition Table syntax:<br>


Was this change intentional?
Look like we have a 'p' tag that is not closed.

johncbaur · 2026-02-21T18:06:07Z

src/Vixen.Modules/App/Props/Models/Arch/Arch.cs

+			LeftRight = false;

-		}
+			DimmingTypeOption = DimmingType.NoCurve;


Shouldn't these Dimming options be initialized in the base class?

johncbaur · 2026-02-21T18:11:07Z

src/Vixen.Modules/App/Props/Models/Arch/Arch.cs

-		[PropertyOrder(11)]
-		public int NodeSize
+		private int _lightSize;
+		public int LightSize


Could this be moved to BaseLightProp ? Seems like all Light props are going to have a Light size.

johncbaur · 2026-02-21T18:13:42Z

src/Vixen.Modules/App/Props/Models/Arch/Arch.cs

-            catch (Exception ex)
-            {
-				Logging.Error(ex, $"An error occured handling Arch property {e.PropertyName} changed");
+		private void Arch_PropertyChanged(object? sender, PropertyChangedEventArgs e)


This event handler can probably be removed.

johncbaur · 2026-02-21T18:30:33Z

src/Vixen.Modules/App/Props/Models/Arch/Arch.cs

-
+
+			// Create Preview model
+			PropModel = new ArchModel();


This class looks like it was updated such that the Arch properties are disconnected from the PropModel properties. The properties on the Arch (prop) can be changed and they won't be reflected on the PropModel. Seems like that could cause problems and was not the original design.

johncbaur · 2026-02-21T19:03:00Z

src/Vixen.Modules/App/Props/Models/IntelligentFixture/IntelligentFixtureModel.cs


 		public MountingPositionType MountingPosition { get; set; }

+		public ObservableCollection<AxisRotationModel> Rotations { get; set; }


We shouldn't have to define Rotations on every prop model. We should define it once in the base class.

johncbaur · 2026-02-22T14:57:01Z

src/Vixen.Modules/App/Props/Models/Arch/Arch.cs

+			Gamma = 2.2;
+			Curve = null;
+
+			ColorTypeOption = ColorType.SingleColor;


Shouldn't the color attributes get initialized in the base class BaseLightProp?

johncbaur · 2026-02-22T15:08:33Z

src/Vixen.Application/SetupDisplay/Wizards/Views/ColorWizardPageView.xaml.cs

+			InitializeComponent();
+		}
+
+		private ColorType ColorTypeOption


I don't understand why this view has properties. Was this cloned from the DimmingWizardPageView? Standard MVVM you bind directly to the view model.

johncbaur · 2026-02-22T15:09:32Z

src/Vixen.Application/SetupDisplay/Wizards/Views/ColorWizardPageView.xaml.cs

+			}
+		}
+
+		private void SingleColorButtonClick(object sender, RoutedEventArgs e)


Normally in MVVM you set up commands and the view model has boolean flags to determine which controls are visible.

johncbaur · 2026-02-22T15:10:32Z

src/Vixen.Modules/Preview/VixenPreview/VixenPreview_xen5ebui_wpftmp.csproj

I don't know what this file is. Seems like it should not be in source control.

johncbaur · 2026-02-22T15:16:27Z

src/Vixen.Modules/App/Props/Models/BaseLightProp.cs

+		Library
+	}
+
+	public enum ColorType


Should we consider using ColorProperty.cs - ElementColorType ?

johncbaur · 2026-02-22T15:23:53Z

src/Vixen.Application/SetupDisplay/Wizards/Views/ColorWizardPageView.xaml

+                      SelectedItem="{Binding SelectedColorSet, Mode=TwoWay}">
+                <ComboBox.ItemsSource>
+                    <x:Array Type="{x:Type sys:String}">
+                        <sys:String>RGB</sys:String>


We should probably create an enumeration for the Color Mixing order we support. I think EnumDescriptionTypeConverter can decorate the enum to provide strings in the WPF UIs.

johncbaur · 2026-02-22T15:27:43Z

src/Vixen.Application/SetupDisplay/Wizards/Pages/ColorWizardPage.cs

+		}
+		private static readonly IPropertyData SingleColorProperty = RegisterProperty<Color>(nameof(SingleColorOption));
+
+		public ObservableCollection<string> ColorSetNames


When doing color mixing (RGB) I think we should consider creating an enum for the color sets / color order we support. This probably means we need separate state for discrete color set names vs RGB, RGBW, color order. The former is entirely user defined. The RGB, RGBW options are determined at compile time and can be represented in an enumeration.

johncbaur · 2026-02-22T15:35:20Z

src/Vixen.Application/SetupDisplay/Wizards/Views/DimmingWizardPageView.xaml

+                    <RowDefinition Height="auto" />
+                </Grid.RowDefinitions>
+
+                <WindowsFormsHost x:Name="ZedGraphControlHost" Grid.Row="0" Grid.Column="0" Width="295" Height="295" Margin="0,3,0,0">


I hesitate to make these comments given I imagine getting this curve stuff working on the Wizard page was probably a lot of work. I commend the effort. Ideally we would have a WPF user control that we drop on this Wizard page that could be reused.

The fact that it does not fit on the Wizard page as it stands is not ideal. The part that off the page could be moved to the right of the drawing area but I am not sure it is worth the effort or to have two versions of the Curve editor in the solution.

Given Jeff views curves as an Advanced feature used sparingly. The Wizard page could just have a button to launch the existing WinForms curve editor and we don't have to touch that code.

johncbaur · 2026-02-22T15:48:31Z

src/Vixen.Application/SetupDisplay/Wizards/Pages/ColorWizardPage.cs

+		#endregion
+
+		#region Public Properties
+		public ColorType ColorTypeOption


Wizard pages are models and don't need to use Catel properties. I think there extra overhead with Catel properties that is not needed for these models and sets a bad example to copy.

jeffu231 changed the base branch from master to feature/VIX-3693 November 29, 2025 16:24

johncbaur suggested changes Dec 1, 2025

View reviewed changes

jeffu231 force-pushed the feature/VIX-3693 branch from b68ff74 to 1f8bd43 Compare January 21, 2026 20:28

da-markson added 9 commits January 28, 2026 17:57

VIX-3697 Added major functionality to support the Arch wizard

796d3b6

VIX-3697 Fixed missing config file that included WPFToolkit

bf0da4e

VIX-3697 Added major functionality to support the Arch wizard

e33f3ca

VIX-3697 Added dimming page into Arch prop wizard.

ac27e19

VIX-3697 Incorporated new OpenGL prop display

6b01550

VIX-3697 Rebased with some temporary work-arounds since I'll be makin…

ed8e0b9

…g major changes

VIX-3697 Phase 1 changes on first page

d605d99

VIX-3697 Curve Wizard page is now working

9934ab9

VIX-3697 Changes now report back to Setup Display dialog

2225876

da-markson force-pushed the VIX-3697 branch from 5a153f6 to 2225876 Compare February 14, 2026 05:29

da-markson added 2 commits February 14, 2026 21:08

VIX-3697 User can now edit existing prop through Wizard

9ebb3ac

VIX-3697 All functionality now works

8d2bef6

jeffu231 reviewed Feb 16, 2026

View reviewed changes

VIX-3697 Bug fixes from testing and code cleanup

ce7c3cb

VIX-3697 Incorporated the final list of changes from the previous PR

fd7661a

da-markson requested review from jeffu231 and johncbaur February 17, 2026 03:37

johncbaur reviewed Feb 20, 2026

View reviewed changes

johncbaur suggested changes Feb 20, 2026

View reviewed changes

johncbaur reviewed Feb 20, 2026

View reviewed changes

johncbaur reviewed Feb 21, 2026

View reviewed changes

src/Vixen.Modules/App/Props/Models/AxisRotationViewModel.cs

Copy link

Contributor

johncbaur Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to move this class back to Vixen.Application ViewModels folder.

johncbaur reviewed Feb 21, 2026

View reviewed changes

johncbaur suggested changes Feb 21, 2026

View reviewed changes

VIX-3697 Implemented Color wizard page

5b51019

johncbaur suggested changes Feb 22, 2026

View reviewed changes

VIX-3697 Updated from design review comments in Confluence

4c385e7



		private ObservableCollection<AxisRotationModel> _rotations;
		public ObservableCollection<AxisRotationModel> Rotations


		#endregion

		#region Temporary properties to get Change Prop to work


		public MountingPositionType MountingPosition { get; set; }

		public ObservableCollection<AxisRotationModel> Rotations { get; set; }

Conversation

da-markson commented Oct 17, 2025

Uh oh!

johncbaur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeffu231 left a comment

Choose a reason for hiding this comment

Uh oh!

da-markson commented Feb 16, 2026 via email

Uh oh!

da-markson commented Feb 17, 2026

Uh oh!

johncbaur Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johncbaur Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johncbaur Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johncbaur Feb 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

johncbaur Feb 20, 2026 •

edited

Loading

johncbaur Feb 20, 2026 •

edited

Loading

johncbaur Feb 20, 2026 •

edited

Loading

johncbaur Feb 21, 2026 •

edited

Loading