Editor: Added samples info for REALISTIC shading #28432
Conversation
|
just in case you can't find it in the demo video 🙏🏻:
|
|
|
||
| samplesText.setValue( samples ); | ||
|
|
||
| const samplesStringKey = ( pluralRules.select( samples ) === 'one' ) ? 'viewport/info/oneSample' : 'viewport/info/samples'; |
There was a problem hiding this comment.
Reconsidering #28245.
Do we really need this distinction? Stuff like that creates an unnecessary complexity. Why not just using Sample(s)?
There was a problem hiding this comment.
Or we just always use the plural? I personally don't see 1 samples as an issue which should be fixed with a separate code path.
There was a problem hiding this comment.
The issue about using (s) for just 'Sample(s)' is that, that unit will be inconsistent with other units formatted in the same area:
1 object
1 vertex
0 triangles
1 sample(s)
The reason that we can't apply (s) for other units has been mentioned #28432 (comment)
If handling of plural rules causes readability problems, then I prefer taking them into a function: (update: this function is now implemented in 54a4957)
function setInfo( quantity, unit ) { // ... set UI text values with proper format ... }
There was a problem hiding this comment.
Then I vote for always using plural (without brackets).
There was a problem hiding this comment.
Note that the transition from 0 > 1 > 2 samples is noticeable in low-end devices, so 'always use plural' like 1samples will confuse users that the quantity is incorrect.
ycw
force-pushed
the
editor-viewport-info-samples-info
branch
from
55ca69b
to
54a4957
Compare
|
Or, we can uses icons with long description as tooltips shown on hover, this way plural rules can be eliminated totally:
Thoughts? thanks :D |
|
Do you mind leaving the plural refactoring for another PR? |
ycw
force-pushed
the
editor-viewport-info-samples-info
branch
from
54a4957
to
bdb29a8
Compare
ok |
|
|
||
| samplesText.setValue( samples ); | ||
|
|
||
| const samplesStringKey = ( pluralRules.select( samples ) === 'one' ) ? 'viewport/info/oneSample' : 'viewport/info/samples'; |
There was a problem hiding this comment.
It seems the plural logic is still in place though....
There was a problem hiding this comment.
Do you mind leaving the plural refactoring for another PR?
^^^^^^^
:D
There was a problem hiding this comment.
I think the current plural code is fine (with the clean up).
The refactoring was over engineering.
|
Some clean up: 1cc0bfe |
|
thanks. |
This PR added samples info in the viewport info for REALISTIC shading:
pathtracer.samples.in.info.mp4
Preview: https://raw.githack.com/ycw/three.js/editor-viewport-info-samples-info/editor/index.html