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

Allow integer slider to support 64 bit ints #10972

Merged
merged 18 commits into from
Aug 26, 2020

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Aug 6, 2020

Purpose

https://jira.autodesk.com/browse/DYN-1085

  • Added IntegerSlider64Bit that derives from SliderBase<long> to support 64 bit integers
  • Deserialize a CoreNodeModels.Input.IntegerSlider type from JSON as a CoreNodeModels.Input.IntegerSlider64Bit so that the new 64 bit slider node is instantiated
  • Serialize 64 bit node back as CoreNodeModels.Input.IntegerSlider type so that new DYN's can be opened by older versions of Dynamo
  • Deserialize 32 bit integer slider from old XML format as new 64 bit integer slider - This does not seem to be straightforward since in order to not break the API I needed to keep the legacy class, which is why I had to retain the legacy behaviour of xml-based 32 bit integer sliders (to be deserialized as 32 bit). Now only when you save the XML file (when saved as JSON) and reopened, will the slider behave as a 64 bit integer slider node, not before.

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

Reviewers

@DynamoDS/dynamo

@mjkkirschner
Copy link
Member

@aparajit-pratap an interesting set of failures, including the ones your predicted.

@aparajit-pratap aparajit-pratap marked this pull request as ready for review August 20, 2020 20:27
@aparajit-pratap aparajit-pratap added PTAL Please Take A Look 👀 WIP and removed PTAL Please Take A Look 👀 labels Aug 20, 2020
@aparajit-pratap aparajit-pratap marked this pull request as draft August 21, 2020 03:47
return BuildIntNode(Convert.ToInt32(value));
case "UInt64":
Copy link
Member

Choose a reason for hiding this comment

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

can we use nameof?

I guess with c# 7 we should be doing:
https://stackoverflow.com/a/4478490

@aparajit-pratap aparajit-pratap marked this pull request as ready for review August 24, 2020 19:25
@aparajit-pratap aparajit-pratap added the PTAL Please Take A Look 👀 label Aug 24, 2020
@aparajit-pratap
Copy link
Contributor Author

@QiltongTang do you know how to change the description of the output port tooltip for a UI node? The only thing remaining in this PR is to change the output port type of the integer slider to report Int64. Int32 is obviously wrong in this case.
image

@QilongTang
Copy link
Contributor

@aparajit-pratap I would try look at when outport model constructor got called when placing integer slider: https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCore/Graph/Nodes/PortModel.cs#L335

@QilongTang
Copy link
Contributor

image

@aparajit-pratap
Copy link
Contributor Author

Thanks @QilongTang!

@QilongTang
Copy link
Contributor

@aparajit-pratap Seems there are regressions with this PR?

@aparajit-pratap
Copy link
Contributor Author

@aparajit-pratap Seems there are regressions with this PR?

@QilongTang there are 2 regressions remaining. I addressed many of them yesterday. Will address the remaining ones today. In the meantime do you have any comments on the rest of the review?

@aparajit-pratap aparajit-pratap requested a review from mmisol August 26, 2020 17:55
Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

Overall looks solid, just a few minor comments!

@aparajit-pratap aparajit-pratap merged commit c1c15d7 into DynamoDS:master Aug 26, 2020
@aparajit-pratap aparajit-pratap deleted the integerSlider branch August 26, 2020 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTAL Please Take A Look 👀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants