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

Fix #4972 Shape center and size not reflective of actual #5143

Merged
merged 37 commits into from
Nov 22, 2022

Conversation

Benbenbenin0
Copy link
Contributor

Merged #4971 into this PR along with #4972

Shapes are now built at center provided by dragging in the design tab, also now displays shapes using the same STL file.

Shape size is now updated to the actual after building from the imported file, however the design tab still leaves default values in the "size [mm]" field that do nothing when changed.

@mkeilman think we need a new issue for that? Not sure whether to let the user scale the STL's size in the designer, remove the field for STL's or do something else.

mkeilman and others added 30 commits September 21, 2022 08:36
@mkeilman
Copy link
Contributor

Shape size is now updated to the actual after building from the imported file, however the design tab still leaves default values in the "size [mm]" field that do nothing when changed.

@mkeilman think we need a new issue for that? Not sure whether to let the user scale the STL's size in the designer, remove the field for STL's or do something else.

We should still display the size but disable the input fields. See

SIREPO.viewLogic('geomObjectView', function(appState, panelState, radiaService, requestSender, $scope) {

It's a one-line fix if you want to add it to this PR, but a new issue is fine too

Copy link
Contributor

@mkeilman mkeilman left a comment

Choose a reason for hiding this comment

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

See comment on centering

Copy link
Contributor

@mkeilman mkeilman left a comment

Choose a reason for hiding this comment

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

a couple minor comments

@mkeilman
Copy link
Contributor

ping @Benbenbenin0 - a couple of changes still in queue here. If you need to focus on ldap that's ok - I'll fix this up; lmk one way or the other

@mkeilman mkeilman merged commit 6ecdf34 into master Nov 22, 2022
@mkeilman mkeilman deleted the 4971-same-stl-vis branch November 22, 2022 21:20
@Benbenbenin0
Copy link
Contributor Author

@mkeilman Thanks for the fixes and merge

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