speckle-sharp icon indicating copy to clipboard operation
speckle-sharp copied to clipboard

Fix bug in SetSpaceLimits.

Open RobClaessensRHDHV opened this issue 2 years ago • 6 comments

Now using BuiltInParameters ROOM_UPPER_OFFSET and ROOM_LOWER_OFFSET for setting LimitOffset and BaseOffset.

Description & motivation

Stumbled upon this issue, not being able to update Spaces within Revit from version 2.16.0 onwards. Checked the code and noticed the problem occurred on the updated lines. LimitOffset and BaseOffset should not be set directly by assigning, but should be updated indirectly with the existing TrySetParam() functionality, targetting the ROOM_UPPER_OFFSET and ROOM_LOWER_OFFSET parameters. I've checked with both Revit 21 and Revit 23 for which it fixes the issue and properly updates all Spaces within the model.

Changes:

Just two lines updated to properly set LimitOffset and BaseOffset of Spaces during SpaceToNative.

Screenshots:

Before - Errors while receiving Spaces (with Revit connector 2.17.0 - problem occurs from 2.16.0 onwards):

image

After - Successfully received Spaces:

image

Validation of changes:

Haven't added tests, but did verify the working in Revit 21 and 23.

Checklist:

  • [x] My pull request follows the guidelines in the Contributing guide?
  • [x] My pull request does not duplicate any other open Pull Requests for the same update/change?
  • [x] My commits are related to the pull request and do not amend unrelated code or documentation.
  • [x] My code follows a similar style to existing code.
  • [ ] I have added appropriate tests.
  • [ ] I have updated or added relevant documentation.

RobClaessensRHDHV avatar Jan 10 '24 14:01 RobClaessensRHDHV

Hey @RobClaessensRHDHV! Thanks for this, I've assigned Connor to have a look at this (though I can't say when this will happen) but meanwhile, we recently started enforcing formatted files on our CI.

For the formatting we've chosen csharpier for now, which is available as a CLI and has plugins for all major IDEs (https://csharpier.com/).

Could you please format the changes in your file using this?

Screenshot 2024-01-11 at 09 38 40

AlanRynne avatar Jan 11 '24 08:01 AlanRynne

@AlanRynne No problem, updated!

RobClaessensRHDHV avatar Jan 11 '24 12:01 RobClaessensRHDHV

Hey @RobClaessensRHDHV,

Would you mind sharing a Revit file and the steps that I need to take to reproduce this issue? For the spaces that I've been trying to update, setting these properties seems to work just fine.

connorivy avatar Jan 17 '24 15:01 connorivy

@connorivy

No problem, here's the file that gives me the issues. Only thing you'll need to do is just send and receive back from that commit / version. Though it does seem to receive without issues, taking a look at the report will show that all spaces aren't fully received. I also made a test stream, you can use it if you like: https://speckle.xyz/streams/3f64444ce4

shared_example_building.zip

RobClaessensRHDHV avatar Jan 22 '24 10:01 RobClaessensRHDHV

@RobClaessensRHDHV

Unfortunately, I am still unable to reproduce the issue that you are experiencing using connector version 2.17.0 and Revit 2021.

I have tried sending "everything" as well as the visible items as a "selection" on floor plans level 1 and 2. I am immediately receiving those back and the spaces seem to be updating correctly. I'm not seeing any errors in the report, and the values for "Limit Offset" and "Base Offset" get set correctly if they were modified.

Even modifying the offset values of the spaces and then receiving the stream that you shared updates the space offsets to the correct value.

I'm not sure what is going on and why you are experiencing an issue while I am not. Do you have any ideas if I am doing anything differently than you? Could you confirm that you are using the same connector and Revit version that I am?

connorivy avatar Jan 23 '24 16:01 connorivy

Hi @connorivy,

Thanks for checking! Very strange that you're not able to reproduce the error. I believe you're going through the same workflow. There's also not much to it, literally sending and receiving some spaces.

I've checked again today, both myself and some colleagues, and we do find the same issue, across different laptops and different Revit versions (21, 22, 23). I additionally checked with a very basic model, basically 4 walls and a space, also giving me the exact same error in the report. I've even checked with old connectors (back to 2.10.4), which give me the same issue. That is somewhat strange as I don't think we have always dealt with this issue. The only thing I can think of as causing this is maybe the exact Revit version. We are running on Revit 2021.1.5, which is somewhat older already. For Revit 2023, we seem to be running on the first 2023 release. On what version are you, and could this possible be the cause of the issue?

Over the different versions and models, the proposed solution still holds. It properly sends and receives, changing any altered parameters on receival.

Additional remark, also the zones don't seem to work as intended. It basically removes my default zones one-by-one on consequent receivals. Anyway, this is a separate issue that I can add to the bug reports.

RobClaessensRHDHV avatar Jan 24 '24 16:01 RobClaessensRHDHV

Hi @connorivy,

I've had an additional check, and the issue still persists. Did you check you're exact Revit version? To me this is currently the only difference I can think of that could explain the peculiar behavior I'm experiencing. I again checked with a most basic model made from scratch, which still gives me the same errors as before.

I updated my branch with the latest upstream commits to bring it back in sync.

RobClaessensRHDHV avatar Mar 26 '24 12:03 RobClaessensRHDHV

Hey @RobClaessensRHDHV ,

I sent your model from Revit 2023 and receiving back in same document seems to be working without any errors.

But receiving in an empty document doesn't seem to work. Which one of the two cases were you describing in your previous reply?

image 0

bimgeek avatar Apr 15 '24 17:04 bimgeek

Hey @RobClaessensRHDHV ,

Ok, trying to receive the commit you shared into your sample file seems to be failing. My guess its something to do with it being from Revit 2021. Can you try sending the same file from 2023 and check if you can receive it back? I was able to send and receive just fine.

bimgeek avatar Apr 15 '24 17:04 bimgeek

Hi @bimgeek,

Thanks for looking into the issue!

I just tried with Revit 2023 also, it gives the same issue as for Revit 2021. Note that you don't need a specific model to try, creating a box-shaped building with one space on the fly, then sending and receiving directly runs into the actual issue. Also note, the actual issue doesn't appear clearly directly, it says Receiving completed as if everything worked, but checking the receive information shows that the Space actually wasn't received properly. See here my one-space-building with the resulting 'error' (partly same error message as in your shared error messages):

image image

It's also clear from the properties within the Space that receival wasn't successful, because the properties aren't updated with values from the received commit as should be the case. Lastly, I again had a check with both Revit 2021 and 2023 to verify that my proposed PR fixes the issue, which is still the case.

If you aren't able to reproduce this issue, then the only (not so plausible) cause I can think of could be the exact Revit version we use. I believe we're not running on the latest patches of Revit 2021 / 2023. I mentioned the exact versions we use earlier in this comment (https://github.com/specklesystems/speckle-sharp/pull/3129#issuecomment-1908428742). If you can't reproduce, it might be worth checking if there could be any important difference between Revit patches.

Hope that further clarifies the issue!

RobClaessensRHDHV avatar Apr 17 '24 15:04 RobClaessensRHDHV

Hi @bimgeek,

Thanks for looking into the issue!

I just tried with Revit 2023 also, it gives the same issue as for Revit 2021. Note that you don't need a specific model to try, creating a box-shaped building with one space on the fly, then sending and receiving directly runs into the actual issue. Also note, the actual issue doesn't appear clearly directly, it says Receiving completed as if everything worked, but checking the receive information shows that the Space actually wasn't received properly. See here my one-space-building with the resulting 'error' (partly same error message as in your shared error messages):

image image

It's also clear from the properties within the Space that receival wasn't successful, because the properties aren't updated with values from the received commit as should be the case. Lastly, I again had a check with both Revit 2021 and 2023 to verify that my proposed PR fixes the issue, which is still the case.

If you aren't able to reproduce this issue, then the only (not so plausible) cause I can think of could be the exact Revit version we use. I believe we're not running on the latest patches of Revit 2021 / 2023. I mentioned the exact versions we use earlier in this comment (#3129 (comment)). If you can't reproduce, it might be worth checking if there could be any important difference between Revit patches.

Hope that further clarifies the issue!

Thanks @RobClaessensRHDHV and apologies on the delay with this PR. We'll take a look and have a WIP out today or early tomorrow.

BovineOx avatar Apr 17 '24 16:04 BovineOx

Thanks @RobClaessensRHDHV and apologies on the delay with this PR. We'll take a look and have a WIP out today or early tomorrow.

@RobClaessensRHDHV just to follow up that that's now merged through and available in 2.18.4. Please let us know if you still have the issue and we'll jump on a call!

BovineOx avatar Apr 17 '24 17:04 BovineOx

Thanks @BovineOx! Happy it's merged. I'll have a quick check later today.

RobClaessensRHDHV avatar Apr 18 '24 06:04 RobClaessensRHDHV

Thanks @BovineOx! Happy it's merged. I'll have a quick check later today.

Works for both Revit 2021 and 2023! : )

RobClaessensRHDHV avatar Apr 18 '24 08:04 RobClaessensRHDHV

Thanks @BovineOx! Happy it's merged. I'll have a quick check later today.

Works for both Revit 2021 and 2023! : )

Perfect! Apologies again @RobClaessensRHDHV. Glad it's working, we've a plan in place to have a better eye on external PRs atm so you should not be waiting anything like as long in future.

BovineOx avatar Apr 18 '24 09:04 BovineOx