Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GridObjectCollection layout: anchor parent to left/center/right and top/middle/bottom #6550

Merged

Conversation

julenka
Copy link
Contributor

@julenka julenka commented Nov 11, 2019

Overview

The current GridObjectCollection will tile the objects with the parent object in the center of the collection - this great if you have a collection that you want to appear in the middle of something, but if you have a UI where you have a grid of buttons, which you want to appear going from the top left of a place and then growing toward the bottom right, this makes things difficult (you need to duplicate the layout logic to figure out where the buttons will be, and then move the parent object to JUST the right location).

This change adds the ability to lay out the buttons so that the parent is in the left/center/right x top/middle/bottom of the collection.

layout

Changes

  • Fixes: Adding tile direction aka "anchor" aka "alignment" support to GridObjectCollection #4777

  • Add a new field named "anchor" to specify how objects should be tiled. Works with all types of surfaces.
    image

  • Adds tests to verify that all anchor layouts tile content correctly

  • When layout type is "Columns then rows", ask user to specify number of columns instead of number of rows.
    image
    image

  • Modify Rows and Columns property so that users cannot change rows if layout type is ColumnsThenRows, and vice versa

Breaking changes

  • If any assets have layout type "ColumnThenRow", update your asset to specify number of columns that should exist in your grid.

To Do

  • Remove breaking change by fixing up asset or offering button on validate to fix incorrect row / columns
  • Add documentation for this change describing anchor. Also add explanations around layout fields

Verification

This optional section is a place where you can detail the specific type of verification
you want from reviewers. For example, if you want reviewers to checkout the PR locally
and validate the functionality of specific scenarios, provide instructions
on the specific scenarios and what you want verified.

If there are specific areas of concern or question feel free to highlight them here so
that reviewers can watch out for those issues.

As a reviewer, it is possible to check out this change locally by using the following
commands (substituting {PR_ID} with the ID of this pull request):

git fetch origin pull/{PR_ID}/head:name_of_local_branch

git checkout name_of_local_branch

@wiwei
Copy link
Contributor

wiwei commented Nov 11, 2019

using System;

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

https://docs.opensource.microsoft.com/content/releasing/copyright-headers.html


Refers to: Assets/MixedRealityToolkit.Examples/Demos/UX/Collections/Scripts/GridObjectLayoutControl.cs:1 in 77406ff. [](commit_id = 77406ff, deletion_comment = False)

Copy link
Member

@radicalad radicalad left a comment

Choose a reason for hiding this comment

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

Love it. Could we mitigate the breaking change by falling back to the row property if the collection is already set up? Perhaps check the layout type and column count and then set it from row?

set
{
anchor = value;
UpdateCollection();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: only UpdateCollection on value change

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same comment applies to the LayoutOrder one too!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought about why the other fields don't call UpdateCollection() after and realized this is because you may want to change multiple properties in a frame, and only need to recompute once. Decided to follow that existing pattern to save on layout compute.

@julenka julenka self-assigned this Nov 11, 2019
@ritijain
Copy link
Contributor

are the AnchorType properties applicable to other surfaceTypes as well ?


Refers to: Assets/MixedRealityToolkit.SDK/Features/UX/Scripts/Collections/GridObjectCollection.cs:19 in 77406ff. [](commit_id = 77406ff, deletion_comment = False)

@julenka julenka added this to the MRTK 2.2.0 milestone Nov 11, 2019
@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@ritijain
Copy link
Contributor

@julenka - PR looks good to me, however I had a few broader concerns regarding the feature -

  1. There is no documentation attached to this PR, going over the usage of the new properties introduced. Having documentation for this is critical since it prevents the exercise of folks trying to checkout the repo and launch the scene or dig through code to discover these properties.

  2. Calling the feature "GridObjectCollection" is a bit misleading, when the script's "Layout" property also support Horizontal and Vertical Layouts. Again the documentation for this is missing (https://microsoft.github.io/MixedRealityToolkit-Unity/Documentation/README_ObjectCollection.html)

I would prefer if 1 above is addressed in this PR. For 2 - in general we would all benefit from opening another issue for improving the documentation of this feature.

@keveleigh
Copy link
Contributor

Looks like .NET validation is failing with

Assets\MixedRealityToolkit.Examples\Demos\UX\Collections\Scripts\GridObjectLayoutControl.cs(108,50): error CS1503: Argument 1: cannot convert from 'string' to 'System.IO.Stream'

@julenka
Copy link
Contributor Author

julenka commented Nov 12, 2019

There is no documentation attached to this PR, going over the usage of the new properties introduced. Having documentation for this is critical since it prevents the exercise of folks trying to checkout the repo and launch the scene or dig through code to discover these properties.

Great point! I had thought maybe it's not needed since the grid object collection docs are already pretty broad, but actually those docs could be improved. I'll add it as a to do for this PR. Thanks for the great feedback @ritijain!

@david-c-kline
Copy link

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@julenka
Copy link
Contributor Author

julenka commented Nov 12, 2019

/azp run

@julenka
Copy link
Contributor Author

julenka commented Nov 12, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@julenka
Copy link
Contributor Author

julenka commented Nov 13, 2019

@cre8ivepark friendly ping. Would especially appreciate your input regarding the layout of the example scene:

image

@david-c-kline
Copy link

@julenka, that scene looks wonderful!

@keveleigh
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@cre8ivepark cre8ivepark left a comment

Choose a reason for hiding this comment

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

Updated the text to use Segoe UI Semibold font, a small adjustment in the layout

2019-11-13 11_18_37-Unity 2018 4 12f1 Personal -  PREVIEW PACKAGES IN USE  - ObjectCollectionExample

@keveleigh
Copy link
Contributor

@cre8ivepark Small comment on your image, but the top right layout color looks kinda like the "missing material magenta". The bottom right one has some outlining it looks like, but I'm not sure about the top right one.

Not sure if it's just the image or something we want to update.

@cre8ivepark
Copy link
Contributor

@keveleigh Oh, it is just spectrum color change when it viewed from a different angle.
2019-11-13 14_06_46-Unity 2018 4 12f1 Personal -  PREVIEW PACKAGES IN USE  - ObjectCollectionExample

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants