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

[API] ImGuizmo #54

Merged
merged 2 commits into from
May 29, 2021
Merged

[API] ImGuizmo #54

merged 2 commits into from
May 29, 2021

Conversation

AAstroPhysiCS
Copy link
Contributor

@AAstroPhysiCS AAstroPhysiCS commented May 8, 2021

This PR finalizes #41 and adds ImGuizmo.

I've already added an example for you to try. There are random weird crashes with the snapping feature and there's still a lot to add, so do not merge it yet.
I am gonna let you know, after I've fixed the crashes and added more features, so that you can go ahead and merge it.
Please have a quick look at the code and let me know if I have any errors/type styles to fix.

Example:

ImGuizmo-Example-java.mp4

@AAstroPhysiCS AAstroPhysiCS changed the title [API] ImGuizmo API [API] ImGuizmo May 8, 2021
@SpaiR SpaiR added 3rd party feat New feature or request labels May 9, 2021
@SpaiR
Copy link
Owner

SpaiR commented May 9, 2021

Hi. Good work! Appreciate the example introduction.
It's problematic to do a proper review at the moment, since your diff is to "noisy". Specifically:

  • removed bin/ dir
  • imgui-binding/tmp/ dir
  • libsNative/ dir

That all creates 65k lines diff and I don't know where to look for real changes. Also, please, check the original PR. I made a small review of it, but I see the same probs here as well.

GIF with example looks good.

@AAstroPhysiCS
Copy link
Contributor Author

AAstroPhysiCS commented May 10, 2021

I've updated the example, deleted the unnecessary build files and fixed crashes.

One thing that I want to note here, is that I am not gonna implement any other features such as ImSequencer etc.
For me, ImGuizmo is enough for now.

I've added comments for you to look and give feedback (in the example file: ExampleImGuizmo.java (row: 262))

PS: Please build the other dll files for yourself. I've only built imgui-java64 since I am running 64 bit Windows.
(using for example imgui-java (32 bit version), you'd get an unsatisfied link error)

Thanks

@AAstroPhysiCS
Copy link
Contributor Author

Apparently, the build for java 8 crashed because I've been using the fancier way of doing switch statements.
I fixed that and this should not be a problem now.

@SpaiR
Copy link
Owner

SpaiR commented May 11, 2021

I'll do a detail review a bit later (maybe on a weekend). What I can see from now is that you don't need to provide bin/ folder at all. I build those libs separatly, so you can skip this part. Also I've noticed you have an _imguizmo.h header file, but you don't use it.

Updated example looks super cool! It's a great addition to the library for sure.

Copy link
Owner

@SpaiR SpaiR left a comment

Choose a reason for hiding this comment

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

And, as I said, please remove bin/ folder.

example/src/main/java/ExampleImGuizmo.java Outdated Show resolved Hide resolved
example/src/main/java/ExampleImGuizmo.java Outdated Show resolved Hide resolved
example/src/main/java/ExampleImGuizmo.java Outdated Show resolved Hide resolved
imgui-app/src/main/java/imgui/app/Window.java Outdated Show resolved Hide resolved
@AAstroPhysiCS
Copy link
Contributor Author

AAstroPhysiCS commented May 15, 2021

I've did more and included pretty much doc for all the necessary methods. It looks professional now and awaits for your approval.
I've deleted the bin/ folder too.
Hit me up if any changes are necessary.

@AAstroPhysiCS AAstroPhysiCS requested a review from SpaiR May 15, 2021 15:37
@AAstroPhysiCS
Copy link
Contributor Author

AAstroPhysiCS commented May 15, 2021

The check failed because there's a unused import in the ImGuizmo class but ImGuizmo class uses the import for the comment. See ImGuizmo (317:71) (probably it is a bug in the check system)
So I guess, your check doesn't check if the import is being actively used in the comments

Other than that, everything checks out.

Copy link
Owner

@SpaiR SpaiR left a comment

Choose a reason for hiding this comment

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

By removing the bin folder I meant to remove it from your diff, not to delete completle. Just revert all changes to the dir.

The check failed because there's a unused import in the ImGuizmo class but ImGuizmo class uses the import for the comment.

Just do the import in the javadoc itself. Like: {@link imgui.extension.imguizmo.flag.Operation#ROTATE}

@AAstroPhysiCS
Copy link
Contributor Author

AAstroPhysiCS commented May 16, 2021

I've reverted all changes to the bin/ folder. Reverted all the changes back. Should be ready now.
I guess, it shows that i changed the binary structure of the files somehow. But i did not, i simply reverted the changes.
I've reverted the folder using checkout command

@SpaiR
Copy link
Owner

SpaiR commented May 25, 2021

For your info, I'll merge the PR on the weekend. Dear ImGui also had released 1.83, so I'll do everything in one go. If you have something to polish - there is time to do that.

@AAstroPhysiCS
Copy link
Contributor Author

I see that you force pushed your own commits in to my master branch. Can I get the reason why you are doing this, since force pushing can produce a lot of mayhem. Additionally, my commits are "deleted" since you force pushed it.

@SpaiR
Copy link
Owner

SpaiR commented May 29, 2021

I prepared the PR for the merge. Your commits are not "deleted", but squashed. I would do it anyway during the merge itself.

Force push doesn't produce a mayhem, it's a normal practice to squash a bunch of commits into one, since it's an atomic change.
Rebases in general are good too. Using of master to commit changes from fork is the bad practice though. I would recommend you to avoid that in future and create a separte branch for you stuff.

@SpaiR SpaiR merged commit 5ea2712 into SpaiR:master May 29, 2021
@SpaiR SpaiR mentioned this pull request May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants