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

Grid: modify default grid and add GUI plugin #125

Merged
merged 1 commit into from
Dec 22, 2021
Merged

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Dec 21, 2021

Breaking this small change from the larger PR so it can be discussed if needed. The default grid is 20mx20m, which is a bit restrictive for our use case. This PR increases it to 100mx100m and leaves the GridConfig plugin available so users can quickly access it to change the grid. I find that useful when measuring distances.

image

@chapulina chapulina mentioned this pull request Dec 21, 2021
11 tasks
@chapulina chapulina self-assigned this Dec 21, 2021
Copy link
Collaborator

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

Looks fine. I haven't really paid attention to the exact optimal default numbers to use, but we can change it in #88 if needed.

Unrelated to this PR, but I think the Grid config plugin needs some UX improvements. Let me know if you agree/disagree and if it's worth a ticket upstream. UX can be very subjective.

  1. What is the scene::Grid(65451) in the drop-down list? That looks like something that shouldn't be exposed to the user.
  2. Horizontal should precede Vertical, since the Vertical dimension is not seen by the user by default, so they're more likely to change Horizontal first to see what it does. Actually, just looking at the screenshot and the code, before running the thing and trying some numbers to see what those two fields do, I totally misunderstood what they did. It's not intuitive that one of them controls both x and y, and the other only controls z.
  3. "Cell Length" can be renamed to be more clear. It's not wrong, but at first glimpse, it is not the most intuitive. There are many lines in the view, so when I see the word length, the first thought is, which length, the longest one? Cell Dimension? I don't know if that's better. Unit Cell Size? It already says Length in the field, so if the boldface title says something else, that will be a second way of expressing the same thing to erase confusions.
  4. "Position" can be renamed to be more clear too. Position of what, a corner, the center, there are so many parts of the grid. The user has to make an assumption, and that means it's not clear. Maybe Grid Center, Grid Origin (that still doesn't clarify it's not a corner), etc.
  5. Color fields would be more customary if they're RGBA vertically, instead of 2x2. That way you don't have to think about whether it's ordered horizontally or vertically.

There is an intermittent rendering problem, where sometimes part of the grid becomes a lighter color. Have you seen that? That's very minor, but seems like something that could be fixed.

@chapulina
Copy link
Contributor Author

Unrelated to this PR, but I think the Grid config plugin needs some UX improvements. Let me know if you agree/disagree and if it's worth a ticket upstream. UX can be very subjective.

It's related! Yes, please ticket something upstream. I want to conduct some proper usability tests by Q2 2022, we'll see if we get to it. Until then, we can improve things based on developer feedback like this.

What is the scene::Grid(65451) in the drop-down list? That looks like something that shouldn't be exposed to the user.

Those are the ugly names that ign-rendering gives the grids. Naming in ign-rendering has been bothering us for years, see gazebosim/gz-rendering#14, which was actually ticketed in response to the first GridConfig plugin I implemented 🙃 For now it is the only way we can tell the grids apart 😕

Horizontal should precede Vertical

+1

"Cell Length" can be renamed to be more clear.

I hear you. I don't know if there's a clear winner for the name though. Right now, that's matching the C++ and XML APIs, which has value in itself. But I guess they all could be changed to something better...

"Position" can be renamed to be more clear too.

+1

Color fields would be more customary if they're RGBA vertically, instead of 2x2.

Yeah I was trying to save space, I think they could fit horizontally 🙂 During the next quarter I want to make a few standard widgets to be reused across plugins, so we make them good once and reuse them everywhere (gazebosim/gz-gui#310). A similar conversation about color widgets happened in gazebosim/gz-sim#1123 (comment), color can be expressed in so many ways 🙂

@chapulina chapulina merged commit 71e0b88 into main Dec 22, 2021
@chapulina chapulina deleted the chapulina/grid branch December 22, 2021 04:53
@mabelzhang
Copy link
Collaborator

Cool, will open a ticket upstream later.

I want to conduct some proper usability tests by Q2 2022

We might get very different results just testing between ROS vs. Ignition developers that do not cross hahahaha. I was actually thinking about systematic studies when this repo gets used by non-developers. If we document this kind of user studies, it would be very valuable for reference later, either internally or publicly. Gazebo-classic had real UX design input right, do we have any of that documented / published somewhere? It would be really good if someone on the Ignition team does a survey of the UX literature, there must be countless, although, who knows, most 3D tools are proprietary.

Those are the ugly names that ign-rendering gives the grids

Is that ign-rendering name exposed to the plugin, so that the XML can give it a name, and we keep that ugly name in the backend, but display the name specified in the XML?

@chapulina
Copy link
Contributor Author

Gazebo-classic had real UX design input right, do we have any of that documented / published somewhere?

Yeah we have some documents from Steffi from back in the day. I found some design specs (spoiler alert, we never implemented them on Gazebo classic, and instead that's what we used for inspiration for Ignition Gazebo), and I remember some guidelines for usability tests that I can't find anymore 😕

It would be really good if someone on the Ignition team does a survey of the UX literature

Definitely, we'll be doing this next year 👍

Is that ign-rendering name exposed to the plugin

Nope, we don't have an API to change the name, and part of it is due to the issue above, ign-rendering still has spots that assume the names are unique and whatnot. We could keep track of custom names within the GUI plugin for example, but that's

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.

2 participants