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

ISSUE #4911 - Milestone 3: Step 3 - Planes height calibration #5069

Merged
merged 81 commits into from
Aug 27, 2024

Conversation

The-Daniel
Copy link
Contributor

@The-Daniel The-Daniel commented Jul 16, 2024

This fixes #4911

Description

  • Adds the vertical plane calibration step for drawings calibration
  • The resultant vectors from the first two steps are used to create a transformation matrix which allows an image of the drawing to be overlayed on a plane in the 3D model
  • The user can set the vertical plane heights either by dragging the gizmo on the 3D model, or using the align to plane feature

Test cases

  • Test different models and drawings
  • Test a range of different calibration vectors
  • When on step 3 go back to steps 2 and 3 and change the vectors

Use the Revit House model (found here), and use the drawings that are title 'rac level1', 'rac level2', and 'rac site'.

#### NOTE: There are two bugs that I need @sebjf to investigate related to re-entering calibration.
When the vertical planes are set and the user cancels calibration and then re-enters the clipping from the previous vertical plane alignment reappears.
Continuing from the previous point, if the user continues calibration again and sets some vertical planes the drawing image appears blank. The height, width, and alignment of the blank rectangle appears correct

The-Daniel and others added 25 commits July 3, 2024 13:59
export const VerticalSpatialBoundariesHandler = () => {
const { verticalPlanes, setVerticalPlanes, vector3D, vector2D, isCalibratingPlanes, setIsCalibratingPlanes, drawingId,
setSelectedPlane, selectedPlane, isAlignPlaneActive } = useContext(CalibrationContext);
const planesRef = useRef(verticalPlanes); // ref needed to get plane values in useEffect without causing excessive retriggers
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you actually need this ref? The places where it's used are the following lines:

  • 106: useEffect dependencies, but useEffect does not detect changes in a ref;
  • 90: you can probably just use verticalPlanes there instead (also, why do you clone the array?)
  • 56: if what said above holds true, this bit becomes useless

Copy link
Member

@carmenfan carmenfan left a comment

Choose a reason for hiding this comment

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

Zane was questioning some of the UX on the demo branch so I had a look here to see if they've been addressed (Most of them are, so thank you 😆 ) but a few more things:

  1. The drawings are appearing to be completely transparent on the background, making them very hard to view, can we make sure we impose a faint white background on it or something like that? very difficult to see at this point
    image

  2. can we only impose the drawing on the plane that is currently selected? have the unselected plane to just display with a mildly transparent white background. The lines overlap with each other and it's very difficult to see
    image

  3. I think we need the clip tool to be available here. If I'm trying to align a level 1 floor plan and I can't get into level one of the floor it's very bad UX. as soon as the bottom plane is established we can reset and disable the clip tool, but before that it will be useful

  4. It's weird that you auto turn on the height calibration tool here, when we don't do that with horiztonal alignment. We should make it consistent.

@sanmont3drepo sanmont3drepo merged commit e1fc566 into ISSUE_4897 Aug 27, 2024
9 of 10 checks passed
@sanmont3drepo sanmont3drepo deleted the ISSUE_4911 branch August 27, 2024 09:00
@sanmont3drepo
Copy link
Contributor

Rest of the problems will be addressed in #5125

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

Successfully merging this pull request may close these issues.

5 participants