Add filter to skip the modifying when the zone of space is null

[ItsPatrickHe]

• "Fix: This PR is made to fix this issue: Revit connector does not import spaces when the zone of space is null #3524

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

[ItsPatrickHe]

@RobClaessensRHDHV tag you in the PR :)

[ItsPatrickHe]

@connorivy Could you check this PR? We need this update in order to receive spaces in the project. Thanks in advance.

[connorivy]

Hey @ItsPatrickHe,

Sorry for the delay on this quick fix. I need to familiarize myself with spaces and zones, but are you aware of what the 'CreateRevitZone' method is doing on line 155? It almost looks like that was the intended path for empty speckle zones (but then it's a bit confusing because we then try to assign it's name from the speckle zone)

[ItsPatrickHe]

@connorivy

Thanks for the reply.

I must say I'm also not that familar with the part of spaces and zones. To me this part seems to either convert the zone in the speckleSpace or create a revit zone if there is no name for the zone. But the last step to assign the name of the speckle zone again is a bit confusing. Could you ask around a bit maybe in your team?

For us this simply will cause an error when you have null for the space zone, then getting name from speckleSpace.zone is impossible.

[RobClaessensRHDHV]

@ItsPatrickHe, @connorivy,

Had a bit of a deep dive in this specific function, which seems to have several issues to it, which I appended in below code:

  private DB.Zone CreateRevitZoneIfNeeded(Space speckleSpace, DB.Zone revitZone, Phase targetPhase, Level level)
  {
    var zoneName = speckleSpace.zone != null ? speckleSpace.zone.name : speckleSpace.zoneName; // zoneName is the previous property retained here for backwards compatibility.
    if (revitZone == null && !string.IsNullOrEmpty(zoneName))
    {
      revitZone = ConvertZoneToRevit(speckleSpace.zone, out _);
    }
    else if (revitZone != null && revitZone.Phase.Name != targetPhase.Name)
    {
      // Deleting this zone is not a good idea, as it potentially still contains lots of other spaces
      // Consequence is that only the first space of this zone will get assigned a new zone, others won't be assigned
      Doc.Delete(revitZone.Id);

      // This conditional assignment seems to be mixed up
      // Namely, if the speckleSpace has no zone.name, you want to create a zone
      // If it does, you want to convert the speckleSpace zone
      // This is currently reverted
      revitZone = string.IsNullOrEmpty(speckleSpace.zone.name)
        ? ConvertZoneToRevit(speckleSpace.zone, out _)
        : CreateRevitZone(level, targetPhase);

      // Assigning a zone name that is potentially null is also not working
      revitZone.Name = speckleSpace.zone.name;
    }
    return revitZone;
  }

I've updated some parts of the function to the below, which at least fixes the issues above. It will only delete the specific Revit Space from the zone, and then only delete the full zone once no spaces are left. The conditional assignment has been reverted and zone name is only assigned if it's actually a valid name.

  private DB.Zone CreateRevitZoneIfNeeded(Autodesk.Revit.DB.Mechanical.Space revitSpace, Objects.BuiltElements.Space speckleSpace, DB.Zone revitZone, Phase targetPhase, Level level)
  {
    var zoneName = speckleSpace.zone != null ? speckleSpace.zone.name : speckleSpace.zoneName; // zoneName is the previous property retained here for backwards compatibility.

    if (revitZone == null && !string.IsNullOrEmpty(zoneName))
    {
      revitZone = ConvertZoneToRevit(speckleSpace.zone, out _);
    }
    else if (revitZone != null && revitZone.Phase.Name != targetPhase.Name)
    {
      // Remove space from existing zone
      SpaceSet revitSpaceSet = new SpaceSet();
      revitSpaceSet.Insert(revitSpace);
      revitZone.RemoveSpaces(revitSpaceSet);

      // Delete zone if no spaces are left
      if (revitZone.Spaces.Size == 0)
      {
        Doc.Delete(revitZone.Id);
      }

      // Convert zone or create new zone
      revitZone = !string.IsNullOrEmpty(zoneName)
        ? ConvertZoneToRevit(speckleSpace.zone, out _)
        : CreateRevitZone(level, targetPhase);

      // Map zone name if existing
      if (!string.IsNullOrEmpty(zoneName))
      {
        revitZone.Name = zoneName;
      }
    }

    return revitZone;
  }

A remaining issue is the fact that it will create new zones for essentially all spaces. In our example, we initially have 15 spaces with the Default zone being assigned. After receiving, all spaces will be assigned with a new, distinct zone as can be seen below. This happens as the revitZone.Phase and targetPhase differ, creating a new zone for each space. It would definitely be more appropriate if one zone would be created for the specific targetPhase, to which other spaces will also be assigned afterwards.

Anyway, I believe it at least addresses some of the issues with the existing code, though it can still be improved.
Let me know what you think!