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

Draft for handling ignored validation warnings #591

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Nov 22, 2024

There has been some feedback saying that the scary, yellow "Validation warnings" indicator, and the message
"Number of warnings: 123"
could be confusing, and raise questions for end users:

Khronos Sample Viewer Warnings

Dramatization via emoji by me

A specific aspect that raised concerns here was that many of the official sample models do generate warnings. And without further context, this could give users the impression that ~"The sample models are not really valid, somehow...". But this is not the case. The sample models are valid. And there are some models that even are intended for testing cases that generate warnings.

However, most of the warnings that had been generated for the "Showcase" models had been
MESH_PRIMITIVE_GENERATED_TANGENT_SPACE
which is described as

Material requires a tangent space but the mesh primitive does not provide it. Runtime-generated tangent space may be non-portable across implementations.

Leaving out the tangent space information can be a conscious decision by the creator of the model (to reduce the size). But it can limit portability in cases where the renderer/viewer does not generate proper tangent space information.


Different options for addressing this have been discussed in the 3D Formats Tooling Working Group. A previous pull request simply disabled this warning on the level of the validator. An undesirable side-effect of this was that

  1. the official sample viewer and the official validator (drag-and-drop tools) behaved differently and generated different reports
  2. there was no indication of this warning any more (meaning that users could think their asset was warning-free, although it isn't)

Another option is drafted (!) here:

When there are "infos", then the number of infos is displayed in a white circle:

Khronos Sample Viewer Info

When there are "errors", then the number of errors is displayed in a red circle:

Khronos Sample Viewer Error

When there are warnings, the special handling kicks in:

When there is any warning that is not one of the "ignored" MESH_PRIMITIVE_GENERATED_TANGENT_SPACE warnings, then the total number of warnings is displayed in a yellow circle:

Khronos Sample Viewer Warning

When all the warnings are the "ignored" MESH_PRIMITIVE_GENERATED_TANGENT_SPACE ones, then it shows an innocent, calming, "i" (for "info") in a Pale Blue Dot.

If there have been "ignored" warnings, then the validator tab will show an elaborate message, saying how many warnings have been "ignored", and explaining the reason why this is OK for the sample viewer. (Wording TBD)

Khronos Sample Viewer Warnings New


The current state of this PR is a DRAFT, to illustrate what this may look like for the end user.

The implementation will have to be cleaned up. And whenever I have to do anything that even remotely involves CSS, I'm reminded why I am not (and will never be) a web frontend developer. But I can do some cleanups, at least (e.g. the change from the previous PR is just commented out here).

@DRx3D @emackey @lexaknyazev

@CLAassistant
Copy link

CLAassistant commented Nov 22, 2024

CLA assistant check
All committers have signed the CLA.

@UX3D-haertl
Copy link
Contributor

Did we come to a conclusion regarding this topic?
Should this PR be merged in the future?
We can also take over work on this PR.

@javagl
Copy link
Contributor Author

javagl commented Jan 30, 2025

I did show this (I think to the Tooling TSG), and there was no "strong feedback". I'll bring it up again tomorrow, just to confirm, but if nobody has strong opinions, then I think this could be a reasonable approach for solving this.

But... the implementation itself may have to be polished/reviewed:

I can do both (probably during the weekend), but am not sure how to handle some of that HTML/CSS stuff.


Rant:

This...
<div style="display:flex;color:black; font-weight:bold; background-color:${color}; border-radius:50%; width:fit-content; min-width:2rem; align-items:center;aspect-ratio:1/1;justify-content:center;">${number}</div>
displays a number with a certain background color

That should be
label.setBackground(color);
label.setText(number);
But maybe it can at least be cleaned up a little bit...

@javagl
Copy link
Contributor Author

javagl commented Feb 1, 2025

We had another look at this in the Tooling TSG, and agreed that it probably makes sense to handle the ignored warnings like this. The last commits contain minor cleanups, but feel free to apply or ask for changes during the review.

@javagl javagl marked this pull request as ready for review February 1, 2025 14:56
@UX3D-haertl UX3D-haertl self-assigned this Feb 3, 2025
@UX3D-haertl
Copy link
Contributor

Currently, if an asset has only the tangent space warnings and info, than the blue i label is shown. Is showing the i indicator more important than showing the number of infos? An example for this behaviour is AnimationPointerUVs.

While I think that using absolute position for the number indicator looks a lot better, it runs into issues with large numbers.
10000 is cut off in desktop mode. On mobile it is already cut off at 100.
From my perspective there are two solutions:

  1. Go back to the old solution, which just centers the indicator and thus allows larger numbers
  2. Use somehting like 100+ for larger numbers and fix the positioning for mobile

@javagl
Copy link
Contributor Author

javagl commented Feb 3, 2025

No strong opinions from my side on this. I thought that showing the validator icon in the background and the number on top looked a bit nicer than just replacing the icon with the number.

I'm not a CSS expert, and don't know how easy each of the options is that I could now brainstorm about:

  • Use the old solution
  • Use something like "100+"
  • Use the new solution, but decrease the font size depending on the number of digits...?
  • Just allocate more space for the number...?

For the question about the importance of the blue (i) vs. the number of "infos":

When the asset contains warnings, then it also shows the yellow "(1)", and not that there may be 100 infos or so. So no information is "lost" with the new approach.

Maybe one option (again, on the brainstorming level) could be to

  • Show the number of errors/warnings/infos (either the upper right, or by replacing the validator icon)
  • Show the blue (i) in another corner (or on top of the number) ONLY when there are ignored warnings

Roughly like this, ...

Khronos Warnings 001

but not done in Paint 😬 and partially depending on how the "large numbers" issue is resolved.

@UX3D-haertl
Copy link
Contributor

Regarding the i indicator: I think the current implementation is fine. Just wanted to double check.

I think the the 100+ (or 999+ fits better I guess) is the best solution since even with the original solution it will look wierd with larger numbers at some point.
I also tried changing the font size, but it quickly becomes tiny and unreadable on mobile.

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.

3 participants