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

Karamba toolkit - Converters for K3D 1D model created #4

Merged
merged 52 commits into from
Mar 7, 2023

Conversation

EnricoAntolini
Copy link
Contributor

@EnricoAntolini EnricoAntolini commented Feb 3, 2023

NOTE: Requires installation of Karamba V3 (pre-release)

Install from:
https://github.com/karamba3d/K3D_NightlyBuilds/releases/tag/3.0.0.4-WIP

Make sure you have the right version by using the Karamba License component:

Description

This PR effectively add translation of Karamba elements to BHoM objects.
In order to Push to external software the converted BHoM objects, some new BHoM_Adapter feature have been implemented. Not all Toolkits have these features already. Currently, you will need:

  • Robot (no action needed -- PR already merged and present in latest installer): it should just work.
  • No other toolkit supported at the moment

Issues addressed by this PR

Closes #5

Test files

  1. Test from @Martian42

  2. Beam cantilever test with Push to Robot: (@IsakNaslundBh could we go through the second bullet point under here together? Could be an example of a BHoM_Adapter issue in the Push of multiple types at same time @alelom)

@alelom alelom self-requested a review March 3, 2023 11:55
@alelom
Copy link
Member

alelom commented Mar 6, 2023

@BHoMBot check required

@bhombot-ci

This comment was marked as outdated.

@bhombot-ci

This comment was marked as outdated.

@bhombot-ci

This comment was marked as outdated.

@EnricoAntolini
Copy link
Contributor Author

At the moment there is an bug in the Karamba3D_Engine_Tests.TestUtilities.BuildModel() that needs an update to a new NuGet version. This will affect only the test and not the converters and is the reason why tests for loads are commented and return a warning for now.

@EnricoAntolini
Copy link
Contributor Author

MicrosoftTeams-image (3)
The ResourceDesigner it is used to handle the .res file where all the warning and error messages are stored. This class is automatically generated and therefore any changes will be deleted every time there is a new change. Would make sense to avoid the compliance checks in this case.

Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Approving based on:

  • Code review: all looks good and makes sense. I would agree with the request for dispensation of the ResourceDesigner file @FraserGreenroyd.
  • Test scripts. All works well now. Tested with Robot, and the current implementation of the base adapter makes this work also with all other structural adapters. Although testing on other adapters will be required, I believe that the current PR holds no responsibility on their working on the BHoM objects output from a Karamba model conversion.
  • Unit test. The provided unit test effectively contribute to the verification of the added functionality. Some improvement is due on some unit tests due to a technical issue with the Karamba Nuget, but this does not affect the functionality, as proven by the test scripts. Therefore, improvements on the unit tests can be done on a separate PR.

@alelom
Copy link
Member

alelom commented Mar 6, 2023

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 6, 2023

@alelom to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 6, 2023

Please be advised that the check with reference 11799346399 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 134 additional annotations waiting, made up of 122 errors and 12 warnings.

@BHoM BHoM deleted a comment from bhombot-ci bot Mar 6, 2023
@BHoM BHoM deleted a comment from bhombot-ci bot Mar 6, 2023
@bhombot-ci
Copy link

bhombot-ci bot commented Mar 6, 2023

The check serialisation has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 6, 2023

This repository is not part of the beta package and does not require a versioning check to be performed.

@bhombot-ci
Copy link

bhombot-ci bot commented Mar 6, 2023

This repository is not part of the installer package, and does not require an installer check.

@FraserGreenroyd
Copy link

Based on chat with @alelom just now, project-compliance failures are ok currently given the set up, and will necessitate changes to Test_Toolkit instead to make use of $(ProgramData) instead of C:\ProgramData going forward to aid non C: Drive developers.

The code compliance, the use of a resx file is currently not compliant to BHoM guidelines from historic set up reasons, and would traditionally be handled with a Query method getting those warnings/errors instead. However, we're evolving BHoM constantly and our move into ZeroCodeTools also supports a change in how we handle certain elements so we can/should have a discussion on this as we end this milestone and go into the next.

For now though, there isn't a requirement for the check to pass until the toolkit goes into alpha (which I know is also desired to happen soon but won't happen in the next sprint due to beta production) so this is fine.

I have requested dispensation for project-compliance and am happy to grant that given the above.

@FraserGreenroyd FraserGreenroyd merged commit a97d459 into main Mar 7, 2023
@FraserGreenroyd FraserGreenroyd deleted the updateToKarambaV3Nuget branch March 7, 2023 10:52
@bhombot-ci
Copy link

bhombot-ci bot commented Mar 7, 2023

FAO: @FraserGreenroyd
@FraserGreenroyd is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is project-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 11799346399

@alelom alelom mentioned this pull request Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L Measured in days type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement 1D elements conversions from Karamba to BHoM
5 participants