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

Revit file sharing #3476

Closed
wants to merge 2 commits into from
Closed

Revit file sharing #3476

wants to merge 2 commits into from

Conversation

adamhathcock
Copy link
Member

Overrides #3468

Uses info from dotnet/sdk#2511

Basically, shared projects are deprecated and only made because of UWP. Using the new build system features makes us more future proof

Copy link
Contributor

@BovineOx BovineOx left a comment

Choose a reason for hiding this comment

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

I think it's fine but @AlanRynne should review also before merging and also we must eb sure it's all good on the CI

@@ -2787,6 +2787,38 @@ Global
{3973D572-5E24-476F-B058-8022D826B793}.Release|Any CPU.Build.0 = Release|Any CPU
{3973D572-5E24-476F-B058-8022D826B793}.Release|x64.ActiveCfg = Release|Any CPU
{3973D572-5E24-476F-B058-8022D826B793}.Release|x64.Build.0 = Release|Any CPU
{FCF934B2-860E-4D2A-AF1A-95B00726AB3B}.Debug Mac|Any CPU.ActiveCfg = Debug|Any CPU
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these or did we just inherit them with new projects?

@@ -0,0 +1,17 @@
<Project Sdk="Microsoft.Build.NoTargets/3.3.0">
<PropertyGroup>
<TargetFramework>netstandard1.0</TargetFramework>
Copy link
Contributor

Choose a reason for hiding this comment

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

1.0? really? but I guess it isn't used

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah could be anything. This csproj is just to see the files independently. Not strictly needed.

@@ -0,0 +1,17 @@
<Project Sdk="Microsoft.Build.NoTargets/3.3.0">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what makes it shared, I guess?

Copy link
Member Author

Choose a reason for hiding this comment

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

This csproj isn't used at all so we don't want msbuild to do anything but can be seen in the sln

}

private Dictionary<DB.ElementId, List<DB.Mesh>> GetTriangulatedMeshesFromSolidByMaterial(DB.Solid solid)
{
var result = new Dictionary<DB.ElementId, List<DB.Mesh>>();
foreach (DB.Face face in solid.Faces)
{
if (!result.ContainsKey(face.MaterialElementId))
if (!result.TryGetValue(face.MaterialElementId, out var materials))
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah.... much better

Copy link
Member

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

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

Got some questions about this before we merge. My understanding is that NoTarget SDK projects are intended for projects that have no code but do have project dependencies (i.e. building it results in no DLL being built, just it's dependencies being copied)
The way we're doing this is somewhat opposite to that 🤔

Comment on lines +36 to +37
<Compile Include="..\Speckle.Connectors.RevitShared\**\*.cs" Exclude="..\Speckle.Connectors.RevitShared\obj\**\*.*"/>
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

I've never had to run this Exclude attribute for the obj folder. 🤔

It kinda feels wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

making a csproj just for shared makes an obj folder.

we don't strictly need the csproj for shared (since each individual "real" csproj includes the files separately) but it's making the shared stuff viewable in the sln

Comment on lines +8 to +15
<ItemGroup>
<Compile Include="**\*.cs" Exclude="obj\**\*.*"/>
</ItemGroup>
<ItemGroup>
<Page Include="**\*.xaml" Exclude="obj\**\*.*">
<SubType>Designer</SubType>
<Generator>MSBuild:Compile</Generator>
</Page>
Copy link
Member

Choose a reason for hiding this comment

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

If the .xaml and .cs files live in this project, why do we need to also add them to the specific 2023 project as well?

Also, that exclude for the obj folder feels wrong 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

they don't really "live" here....it's not used by other csprojs if you look

@adamhathcock
Copy link
Member Author

Got some questions about this before we merge. My understanding is that NoTarget SDK projects are intended for projects that have no code but do have project dependencies (i.e. building it results in no DLL being built, just it's dependencies being copied) The way we're doing this is somewhat opposite to that 🤔

You're right....the shared csprojs could be deleted and we wouldn't need to exclude the OBJ folder as it wouldn't exist. The shared csproj is just for viewing in the SLN

files are directly added to each csproj via the compile include

@adamhathcock
Copy link
Member Author

went through this again with @AlanRynne in some detail and actually gives a slightly worse DX even though shprojs aren't supported going forward. Taking this on the chin for now.

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

Successfully merging this pull request may close these issues.

3 participants