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

helix upgrade #14134

Merged
merged 20 commits into from
Aug 7, 2023
Merged

helix upgrade #14134

merged 20 commits into from
Aug 7, 2023

Conversation

marbelarrietaGlobant
Copy link
Collaborator

@marbelarrietaGlobant marbelarrietaGlobant commented Jul 6, 2023

Purpose

  • updates to helix 2.24.0
  • recompiles shaders
  • updates hlsl common classes that our shaders reference
  • move visualization tests from /src to /test
  • removes all net48 conditional code and references related to helix.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@sm6srw
Copy link
Contributor

sm6srw commented Jul 6, 2023

I see this failure on the 4.8 build: E:\Builds\Dynamo_master\Dynamo\src\DynamoCoreWpf\DynamoCoreWpf.csproj(1722,3): error MSB4025: The project file could not be loaded. The 'ItemGroup' start tag on line 134 position 4 does not match the end tag of 'Project'. Line 1722, position 3

@marbelarrietaGlobant
Copy link
Collaborator Author

I see this failure on the 4.8 build: E:\Builds\Dynamo_master\Dynamo\src\DynamoCoreWpf\DynamoCoreWpf.csproj(1722,3): error MSB4025: The project file could not be loaded. The 'ItemGroup' start tag on line 134 position 4 does not match the end tag of 'Project'. Line 1722, position 3

@sm6srw I can't replicate that, actually DynamoCoreWpf.csproj doesn't have a 1722 line. Can you please give me more details?

@sm6srw
Copy link
Contributor

sm6srw commented Jul 7, 2023

@marbelarrietaGlobant I have only seen it on one of the build machines. Can you resolve the conflicts and push again? Thanks!

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

⚠️ [run-bin-diff] - Files Added/Deleted::4 new file(s) have been added
⚠️ [run-bin-diff-net60-windows] - Files Added/Deleted::5 new file(s) have been added and 2 file(s) have been deleted!
(Updated: 2023-07-27-19:44:15)

Copy link
Contributor

@sm6srw sm6srw left a comment

Choose a reason for hiding this comment

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

LGTM!

if (Environment.Is64BitProcess)
NativeMethods.LoadNvApi64();
else
NativeMethods.LoadNvApi32();
Copy link
Contributor

Choose a reason for hiding this comment

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

so we have 32 bit support here just because this code has been copy pasted right >?

Copy link
Member

@mjkkirschner mjkkirschner Aug 7, 2023

Choose a reason for hiding this comment

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

yeah, I also asked about this class being removed here:
helix-toolkit/helix-toolkit#2004 ... so - not sure it will work.

you can give this blog post on a related tool a read if you are interested -
https://www.toptensoftware.com/blog/nvpatch-how-it-works/
though it seems we might be able to do something now that .net supports native external callers ...

anyway, that blog post and this method don't seem to work the same way so there must be more to it than an exported member in the pe file. I guess?

For the time being, I think this is safe enough even if it does not work - we'll just have to pay attention to reports of users needing to explicitly set hardware accel again for sandbox - like back in the dark ages 😉 )

Copy link
Contributor

@pinzart90 pinzart90 left a comment

Choose a reason for hiding this comment

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

one question about NVidia native binary import then LGTM

@mjkkirschner mjkkirschner merged commit 644366c into DynamoDS:master Aug 7, 2023
14 checks passed
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.

5 participants