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

Pointcloud #36

Merged
merged 24 commits into from
Jul 27, 2022
Merged

Pointcloud #36

merged 24 commits into from
Jul 27, 2022

Conversation

jasonpywell
Copy link
Collaborator

Pull request to merge the point cloud branch into the main branch

Copy link
Collaborator

@j-w-moebius j-w-moebius left a comment

Choose a reason for hiding this comment

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

Nice work!
Seems to work fine, except for the interaction with vertex color: If vertex color is set, the point cloud appears in black. One easy fix would be to simply switch off vertex color at each point cloud conversion via self.control.vertc.set(False).

Copy link
Collaborator

@AlexanderRitter02 AlexanderRitter02 left a comment

Choose a reason for hiding this comment

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

Ok, review added, looks good.
Couldn't run it yet , but I looked through the code and adressed a few things like coding style and naming.
Please excuse the brief comments, I didn't have much time.

Also, pay very close attention when fixing the merge conflicts that the program keeps working, because some commits overlap.

Comment on lines 3 to 4
def DoNothing(self):
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent work on this function! xD

pointcloud/point.py Outdated Show resolved Hide resolved
@@ -31,4 +31,5 @@

root = tk.Tk()
my_gui = ProgramGUI(root)
my_gui.pack(fill=tk.BOTH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

When you get to fixing the merge conflict before merging this PR, make sure to NOT use this code, but instead the code using grid from main.

(This happened because I cherrypicked this commit because I needed it and later changed it)

gui/gui_main.py Outdated Show resolved Hide resolved
CreatePointcloudFromObject.py Outdated Show resolved Hide resolved
Comment on lines 81 to 82
def convert(self):

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename this function to be named more meaningful, convert can convert anything and doesn't tell that it's for pointclouds.
This is especially important because you import it as a non-prefixed name in gui_main.py.

@AlexanderRitter02 AlexanderRitter02 added the enhancement New feature or request label Jul 25, 2022
@AlexanderRitter02 AlexanderRitter02 linked an issue Jul 25, 2022 that may be closed by this pull request
@jasonpywell jasonpywell merged commit de67092 into main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to add plane benath object
3 participants