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.

Sign up for GitHub

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

Jump to bottom

Add the IFC dotnet converter #3525

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

Add the IFC dotnet converter #3525

wants to merge 51 commits into from
+171,771 −171,615

Conversation

adamhathcock
Copy link
Member

No description provided.

iainsproat
iainsproat requested changes Nov 20, 2024
View reviewed changes
packages/fileimport-service/Dockerfile Outdated Show resolved Hide resolved
packages/fileimport-service/Dockerfile Outdated Show resolved Hide resolved
packages/fileimport-service/Dockerfile Outdated Show resolved Hide resolved
packages/fileimport-service/Dockerfile Outdated Show resolved Hide resolved
packages/fileimport-service/src/daemon.js Outdated Show resolved Hide resolved
packages/fileimport-service/Dockerfile Outdated Show resolved Hide resolved
@adamhathcock adamhathcock changed the title Adam/ifc dotnet Add the IFC dotnet converter Nov 21, 2024
@adamhathcock adamhathcock marked this pull request as ready for review November 21, 2024 13:05
iainsproat
iainsproat previously approved these changes Nov 21, 2024
View reviewed changes
@adamhathcock
Copy link
Member Author

@iainsproat I think I fixed everything about docker....need confirmation of the corepack change

@github-actions GitHub Actions
Copy link
Contributor

📸 Preview service has generated an image.

@github-actions GitHub Actions
Copy link
Contributor

📸 Preview service has generated an image.

@github-actions GitHub Actions
Copy link
Contributor

📸 Preview service has generated an image.

@adamhathcock
Copy link
Member Author

Two of the example files are failing on the step parser. I think it's because of whitespace/separator intolerance.

@iainsproat
Copy link
Contributor

iainsproat commented Nov 28, 2024 •
edited
Loading

@adamhathcock - do we have a better way of managing/importing wasm files, so that the provenance can be known and followed by a community member? #3525 (comment) edit: discussed and this was an existing file, so we will keep as is

If the two example files can be shown to pass the revised IFC parser, I'd be happy to approve for merging. We can then enable the feature flag on latest and have the wider team test it.

We still need to investigate the file provided via speckle.community which was causing high memory usage. I don't consider it a blocker to merging though. It was reported as working on the original node js IFC parser if its memory limit is increased to 4Gi. What upper bound of memory is required for the .Net parser? Is there a memory leak or other memory-usage inefficiency we can resolve?

@github-actions GitHub Actions
Copy link
Contributor

📸 Preview service has generated an image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iainsproat iainsproat iainsproat left review comments

@gjedlicska gjedlicska gjedlicska requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adamhathcock @iainsproat @gjedlicska