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

WIP: Feature/refactor #62

Merged
merged 24 commits into from
Oct 6, 2023
Merged

WIP: Feature/refactor #62

merged 24 commits into from
Oct 6, 2023

Conversation

it-ony
Copy link

@it-ony it-ony commented Sep 22, 2023

Hi Peter,

still WIP, but worth going through the refactoring commit by commit. I try to make the code more readable by splitting them into smaller junks.

We can also pair on this in a session.

Best,
Tony

@pludikar pludikar self-requested a review September 23, 2023 02:13
@pludikar
Copy link
Collaborator

pludikar commented Sep 23, 2023 •
edited
Loading

Hi Tony,
I'm not sure that the run and stop methods can be moved to the command. F360 is expecting to see those functions and not an object.

I use dogBone as a prefix in a vague attempt to avoid confusion - I'm okay with cleaning it up, though

typing: I don't think it's necessary to import cast. Fusion has a .cast method on most of it's methods. I've stopped using that because python provides a simpler annotation:
_design: adsk.fusion.Design = _app.activeProduct
the annotation and cast basically do exactly the same thing - helps the IDE to autocomplete/prompt

todo: I want to use python standard libraries that doesn't require users to do anything. F360 allows you to put libraries into a specific directory, and the python interpreter will find them. but... it requires the user to install the libraries in the right place. The F360 python environment is also very fussy about which directories are searched for modules. Unfortunately the only way I could get a library in a subpath to be found is to insert it into the environment path - IDE inspections have to take a back seat unless you can find a way around it.

I don't think the "with" python functionality existed when I first wrote the add-in (I think it was 3.7, if I remember correctly). I didn't get around to updating it - so that will work. However, dBParameter is a dataClass, so json to and from it is an inherent method to that class. You need to make sure that you don't break it by changing it too much.

Otherwise it looks great so far. Thanks

Peter

@it-ony
Copy link
Author

it-ony commented Sep 23, 2023

I'm not sure that the run and stop methods can be moved to the command. F360 is expecting to see those functions and not an object.

There are still global run and stop functions available, which call the instance methods.

_design: adsk.fusion.Design = _app.activeProduct
The IDE actually warns about this, as def activeProduct(self) -> Product: returns a Product and adsk.fusion.Design is one of the possible classes inheriting from Product.

The cast is basically ensuring that the runtime type is reflected during inspection time. Same is true for places, like this

cast(adsk.core.SelectionCommandInput, input.parentCommand.commandInputs.itemById(
                FACE_SELECT
            )).hasFocus

where the IDE then understands that hasFocus is available, vs. without the casting, we only have a CommandInput at our fingertips from def itemById(self, id: str) -> CommandInput, which doesn't have hasFocus.

I want to come to a point, where the IDE can detect all potential errors. Right now warnings are all over the place making it hard to spot bugs.

E.g.

[selectedEdge.select for selectedEdge in self._associatedEdgesDict.values()]

might need select(), but I cannot say at the moment.

todo: I want to use python standard libraries that doesn't require users to do anything. F360 allows you to put libraries into a specific directory, and the python interpreter will find them. but... it requires the user to install the libraries in the right place.

I'm with you. But there are other ways to achieve this. I have it on my list as part of make the IDE understand the code better.

However, dBParameter is a dataClass, so json to and from it is an inherent method to that class. You need to make sure that you don't break it by changing it too much.

Yes. I'll come to a point where the functions are small enough to actually be tested as part of a CI system. Currently its impossible.

@pludikar
Copy link
Collaborator

pludikar commented Sep 23, 2023 via email

@it-ony
Copy link
Author

it-ony commented Sep 23, 2023

Which IDE are you using?

Intellij Idea from jetbrains, so same as pycharm

@pludikar
Copy link
Collaborator

pludikar commented Sep 23, 2023 via email

@pludikar
Copy link
Collaborator

pludikar commented Sep 24, 2023 via email

@it-ony
Copy link
Author

it-ony commented Sep 24, 2023

Hi Peter,

less warnings in VSC only means, that it doesn't inspect the code that deep. As already pointed out
_design: adsk.fusion.Design = _app.activeProduct might work in VSC without a warning, but as def activeProduct(self) -> Product returns a Product and adsk.fusion.Design is one of the possible classes inheriting from Product intellij is actually correct about this and checks, that assignments are correct.

Just so you know - the changes you made to the import break the code. F360 is very fussy about where it finds imports from, and updating the system path with the subpath is the only work around that I know of. That means the short piece of code at the start of the Dogbones.py inserting the subpath MUST be before the import of dataclasses and pydantic, otherwise dbclasses and dbdata will not find the appropriate library.

Yes, I noticed this. It's a problem of only manual testable code. In my ongoing changes I reverted it. I hope I can provide a better version this evening.

Tony

@pludikar
Copy link
Collaborator

pludikar commented Oct 1, 2023

Hey Tony,
If you are still interested, I've found a cure to the dogbone placement inconsistency. It now appears to work as I expected, and as I'm sure it did before F360 messed around with their code base. I've not merged it yet, but you'll find it on the bug-fixing branch. There's still an issue with the manufacturing Model edit, but if you save, close and open again it works. I'll be working on that over the next few days. I still need to check the parametric dogbones. I wanted to get the fix to you, to give you confidence that it works.

I'm also going to cherry-pick most of your changes, particularly the ones that don't effect the import search path. I'll upload the cabinet model you sent me, and I'll send you the link to your private email.

Peter

@pludikar
Copy link
Collaborator

pludikar commented Oct 6, 2023

Hi Tony,
Sorry if I messed up the merge of your PR. I propose to change the git history - I have my proposal here.

In it I have included all your PR commits (except for the import changes). I have also included my bug fixes for both the static and parametric dogbones. If you rebase your PR to the head, I believe everything should tie back to your changes without significant conflicts (just the 3 commits to correct the bugs)

If you're ok with my proposed git tree editing, then I'll do a "push --force" to affect the changes on the main repository. If you have a better idea, please let me know

Peter

@it-ony
Copy link
Author

it-ony commented Oct 6, 2023

Hi Peter,

I see too many changes and I get too many conflicts. Also from the 24 commits, I only see a few, so I believe we were lost in the communication in between, e.g. the imports have been corrected in one of the early commits of the PR.

if you remember from our conversation, my goal was to get Dogebones to a state where they can be updated even if fusion doesn't support this. As the structure of the code was not supportive to implement the update feature, I came up with refactoring and this PR. Even with my proposed changes the code was still in a state where it's hard to implement the update command.

Thats why I started with a new plugin from scratch using a structure similar to the finger joint plugin. It worked out and in a few hours, I was able to create dogebones with your algorithm, plus they can be updated. Same is true for the finger joints plugin with this PR

Not all features have been ported, e.g. minimal and mortise dogebones, but it's relative easy to add this as the logic happens in a single place and not in the UI/inputs.

Given that state, I think we can close this PR and not merge it. I would appreciate, if you could take a look to the plugin. If you like the structure, we could work together on porting all the features that are missing and then bring this code back to your original repo. If you're happy with the current state of the plugin, we keep 2 forks and I'll add the features I personally need to the new one.

Credits to your work have been given.

Thanks,
Tony

@it-ony it-ony closed this Oct 6, 2023
@pludikar
Copy link
Collaborator

pludikar commented Oct 6, 2023 via email

@pludikar pludikar reopened this Oct 6, 2023
@pludikar pludikar marked this pull request as ready for review October 6, 2023 21:30
@pludikar pludikar merged commit a4342a9 into DVE2000:master Oct 6, 2023
@pludikar
Copy link
Collaborator

pludikar commented Oct 6, 2023

Hey Tony,

I've merged your PR - I reset the head to before my revert and pulled in all your commits. I'll now add in the bug fixes I found and commit them later.

Peter

@pludikar
Copy link
Collaborator

pludikar commented Oct 7, 2023 •
edited
Loading

Hi Tony,

For the most part your refactoring is great. I like it.

I originally left params as a instance method of Dogbone class because I wanted to make each dogbone instance individually settable - I was planning to emulate features such as Fillets, where you can set different Fillet parameters for different sets of edges.

I was also aiming to make each document (model tab) a separate instance of DogboneCommand. I often have more than one model on the go at a time (particularly while testing - but if it works under those conditions, it should work robustly under normal conditions), and I don't always want the same parameters for one model to be the same as others. The AddIn is global to F360, and ideally should be isolated.

During my debugging process over the last couple of weeks I found that the global nature of the addin has resulted in the GC hanging references and not cleaning up. To force a clean set of variables on DogboneUI instantiation (old OnCreate), I've had to specifically reset all the variables you now have in Selection. At the moment self.selection = Selection() is pre-populated with old data. There's obviously a hanging reference somewhere that either needs to be deleted, or the variables need to be reset.

Peter

@pludikar
Copy link
Collaborator

pludikar commented Oct 7, 2023 via email

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

@pludikar pludikar Awaiting requested review from pludikar

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
@it-ony @pludikar @tonfido