Measures support (with toggle button) #819
Conversation
for more information, see https://pre-commit.ci
|
Preview PR at appsharing.space |
|
Integration tests report: appsharing.space |
There was a problem hiding this comment.
Thanks @asmith26, this is awesome work.
However i have some minor comments that can be addressed before merging.
Other than that I notice that when we select an edge - it's a 2D plane instead of a bounding box and any of the x, y or z value is 0. Should we hide that label whichever value is 0?
Refer to this image:
| const material = new THREE.LineDashedMaterial({ | ||
| color: 0x000000, | ||
| linewidth: 1, | ||
| scale: 1, | ||
| dashSize: 0.1, | ||
| gapSize: 0.1 | ||
| }); | ||
| const geometry = new THREE.BufferGeometry().setFromPoints([start, end]); | ||
| const line = new THREE.Line(geometry, material); | ||
| line.computeLineDistances(); | ||
| this._group.add(line); |
There was a problem hiding this comment.
What is the significance of these dashed lines? I ask because I see the measurements working similarly without these too.
There was a problem hiding this comment.
I've found these helpful when measuring multiple objects, e.g.
or between objects (e.g. selecting the 2 edges):
Of course happy to remove if preferable?
There was a problem hiding this comment.
I like the dash lines, actually I like them so much that I think they are not visible enough 😄 Could we make them more visible when measuring one object?
There was a problem hiding this comment.
+1 on keeping the lines. I haven't been able to properly notice them before as they aren't visible enough. Thank You for the justification :)
There was a problem hiding this comment.
Thanks for the feedback, what do think of 9329bbe
|
About the UI tests, they just seem a bit flaky to me after i inspected the snapshots. |
I've tried to fix the failing ones, and they seem to be passing now. |
Great thought thanks, I like this idea as it could help to keep the UI cleaner. Will have a go at implementing this (hopefully) tomorrow! |
asmith26
force-pushed
the
measures_support_ruler
branch
from
2c94c46 to
cae7af3
Compare
|
In the latest version, I notice that you have also managed to fix:
Cheers 🎉 |
|
One thing that I noticed while testing the PR: When an object is aligned to the 
However when the object is tilted using the transform controls, the dashed lines are no longer aligned with the bounding box of the selected object: 
We discussed this a bit internally, we think that the dashed lines should be aligned with the bounding box of the object only when measuring a single object. However, since this PR adds also adds support for multi object measurement - the dashed lines should be aligned with the world axes in other cases. Sorry if this sounds like a lot, we would be happy to help at any point. |
for more information, see https://pre-commit.ci
Great idea, added in 4abf570
No problem at all, I'm happy to help with this amazing project in anyway that I can! |
Added in 56fdfca |
|
Thanks very much for all the feedback, I think I've implemented everything, although I'm unsure as to why the JupyterLite tests are now failing? |
3 participants
Please see comments in #818 for main functionality added - this PR includes the ability to toggle on/off the measurement feature.
Original comment:
Fixes #124
measure1.mp4
measure2.mp4
(This highlights the dimensions are actually represented with a dashed line - we can see this by removing a selected object:
measure3.mp4
I have noticed that when transforming a shape the dimension labels don't move with it, but deselecting and re-selecting it correctly updates the location (but I could try and get the labels to move with it if you think this is possible and would prefer it):
measure4.mp4
Very much welcome any thoughts: I did wonder if would we like to have a button to toggle measurements, instead of always showing them whenever an object is selected (I think this might also fix the broken snapshot UI tests)? Thanks!