Hi,
I had a recent request to add declarative support for GUI 2D Grid control. So, I’m adding row and column definitions and then grabbing the definition for possible future updates:
grid.addRowDefinition(props.height, props.isPixel);
const valueAndUnit = grid.getRowDefinition(grid.rowCount - 1)!;
Now if that was consistent with other parts of the framework then I would be able to set those properties. ie: valueAndUnit.isPixel = false, but that yields an error:
Cannot set property isPixel of [object Object] which has only a getter
I can see why after checking the code is that I would need to call updateInPlace with the new values. I think this is a question for @msDestiny14.
I have worked quite a bit on MVVM and other GUI patterns that encapsulate change logic, so I would have thought that the ValueAndUnit would have published a “changed” event that would automatically have the control(s) using it to mark themselves as dirty and a Composite pattern to handle that transparently. Considering only I am asking this (didn’t see anything in forum search) then maybe it would not be a useful feature. One thing, for example, is that the fluent/builder design of the API makes it a bit more work to access the ValueAndUnit and it would appear more the intent is that most people leave those unchanged? Any thoughts on grids resizing automatically by setting the named properties (isPixel, value)?
3 Likes
This is a very good point and observation. In fact it was quite a challenge when it came to the GUI editor itself because you are able to modify the the grid definitions in real time. We also have a sort of “temporary” state array of values for when the user types in new definitions but has not confirmed them yet. Once they are confirmed I go through each call setRowDefinition or setColumnDefinition with the new values entered. Internally this will just create a new ValueAndUnit and rewrite the previous.
As for the ValueAndUnit class. This is used in multiple places in our code, not just grid…
I’m actually really thinking about this one. 
Why wouldn’t we create a callback that that notifies the user of ValueAndUnit “hey I changed, mark yourself as dirty” But this would require a lot of hook ups in the GUI system. I can see a world that we do this? Make it optional so it’s not constantly sending notifications to nothing. Is this something you’d like to push for? For grid I could see it working, what about other controls?
Keep in mind why we have ValueAndUnit in the first place. In my mind it’s to abstract the user, whether they type “40px or 40%” this will return render the desired value, In the GUI editor for example, the system go through some extra lengths before the user even finishes typing. For example if you only type ‘40’ we will auto fill to px or % based on if the editor mode is in “responsive” or not.
I’d love to hear more thoughts if you have them. This one made me think. 
1 Like
hi @msDestiny14 thanks for your thoughts as well! It was interesting to read about your experience with the editor. I’m sure if there was nothing else to do it would get done as it’s not “difficult” to add!!
I think the Grid is great and I’m glad it was added! This change is not something I would like to push for - mostly an observation as it is already done in other parts of the GUI already and absent when ValueAndUnit is used. Here is a setter on a primitive that is intuitive to use and updates transparently:
Babylon.js/inputText.ts at master · BabylonJS/Babylon.js (github.com)
It looks like it is not being done on ValueAndUnit objects, because there is no easy way to detect that the values have changed without polling and tracking. I don’t see a problem at all with constantly sending notifications to nothing - that’s the nature of pub/sub design and I suspect would not be a real issue. Event driven design like the built-in observables create a clean decoupling.
It feels to me like you kind of answered the question yourself when you said it was quite a challenge to modify grid definitions! With this in place it would have been (at least in my case) trivial to wire up that responsiveness to row/column width/height. Needing to dig into internals like rebuilding the rows/column to me is a sign there is room for improvement 
In my case also I can’t use setRowDefinition easily because I would need to track or find the columnIndex. It would be very easy in JSX to hide columns declaratively even in the “middle” of the table, so the column index would also not match. The only way I can think of to get the correct index would be to loop through with getColumnDefinition() for all columnCount and do object strict equality test (the grid definition arrays have no public accessors). When I came to that realization and dreaded writing all that code to change a column width was when I asked the question. 
1 Like
Would a PR be accepted on this? I started a draft here:
add ValueAndUnit change tracking · brianzinn/Babylon.js@9bf8875 (github.com)
I can finish the tests and I also would like to rename the constructor variable name to match the property. Is this where GUI tests go - are there none or I just couldn’t find them? The other thing that I noticed is that negativeValueAllowed is only enforced in fromString, so I think that it should be checked in other setters. Cheers.
2 Likes
I have no immediate objections, but I have a couple of cautions and questions.
- Make sure this is backwards compatible.
- I see you have added stuff to the testing section. I am less familiar with our automated tests for this system but making sure you are passing any existing tests, writing additional tests, and then testing those seem like a great idea.
- Taking another glance I see you have added that observable. Should this be optional?
I think further discussions on this will be best on GitHub with a PR. 
Here is a PG for anybody curious the underlying reasoning behind the change request:
Animated Grid | Babylon.js Playground (babylonjs.com)
3 Likes