-
Notifications
You must be signed in to change notification settings - Fork 1k
OpenGL / 3D view refactor #1498
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
base: master
Are you sure you want to change the base?
Conversation
|
Hello @DivingDuck
No worries, there is no rush! Your additional information helped me to pin down the problems a bit further.
I was able to reproduce this when I scale down the 3d view to a very small window. I believe zoom not working was caused by a minimum and maximum zoom distance, that I added to the code. I turned this off and hope this will solve this issue.
After looking into this I found out that I didn't set the current context before loading a new 3d model. I hope this is fixed now.
Yes, the grid had a hard coded colour before. I now changed it in a way that it automatically becomes light grey or dark grey, depending on the colour of the background. Implementing a user picked colour can also be done, if we want that.
Yes it is not very good, but it should behave the same way a before. Implementing a better algorithm was not on my todo list for this PR. Too many small details. So, feel free to test the new binaries, when you find the time. I am optimistic that this two points have been fixed now. |
|
Hi @neofelis2X,
Solved.
Yes. Solved too.
Saying this, I recognize again that we should rework the whole setting story. Is it only me who is stumbling all the time in finding out where a specific default or fix setting was made in our code? I think we should have a central place for those values. Maybe worth or a future project.
Don't worry, this is somehow as well much better than in my last test. It looks like the zoom problem had forced this glitch. Edit: You fixed the grid issue already. 👍 |
|
Hi @neofelis2X, The way I force this: Open Pronterface, go to -->Settings -->Options -->Viewer and then for [3D viewer options] check / uncheck both settings for [Use a lighter 3D visualisation] and [Use perspective view instead of orthographic] and then hit the Ok button. Edit: |
|
Hi @DivingDuck, so I made some changes to avoid that error.
Great, I am glad that this is fine now. I messed up the colour calculation, but fixed it shortly afterwards. I will see if I can add the grid colour to the settings.
Yes, I absolutely agree. The settings are confusing and I try to avoid messing with them, when I can. This specific error you mentioned can be triggered in many different ways, especially when multiple settings are changed at the same time. The way Pronterface applies changed settings is brute force, it sometimes calls To improve this a little bit I changed 2 things:
As you say, generally the whole settings system should probably be reworked. I never quite understood the flow of actions when changing a setting. |
printrun/gl/keyboardinput.py
Outdated
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this line necessary? Are we ever running this file as a standalone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, especially in this file it is actually not necessary.
| # BASE_CLASS = wx.Window | ||
| # Subclassing Panel solves problem In Windows | ||
| BASE_CLASS = wx.Panel | ||
| # BASE_CLASS = wx.Panel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive my ignorance here, if the focus issue is solved for all operating systems, could we remove this line instead of commenting it out? In fact could we remove the other comments and just leave a comment on why we subclass GLCanvas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the comments in the pyglet2 branch. I leave them in here for now, in case anything comes up. Can't really test linux systems that much.
printrun/gl/panel.py
Outdated
| self.pygletcontext.canvas = self | ||
| self.pygletcontext.set_current() | ||
| # Uncomment this line to see information about the created context | ||
| # print(f"OpenGL context version: {self.pygletcontext.get_info().get_version()},\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put this on a logging.debug call? Or would it mean too much clutter?
printrun/gl/panel.py
Outdated
| self.focus.update_size() | ||
| self.OnInitGL(call_reshape = False) | ||
| # print('glViewport', width) | ||
| # print('glViewport: ', width, height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this print comment? Use a logging.debug kind of message instead?
printrun/gl/actors.py
Outdated
| # glCallList(self.display_list) | ||
| self.draw() | ||
|
|
||
| ''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these apostrophes for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it... I will just leave a TODO and delete the rest of the old code.
printrun/gl/actors.py
Outdated
| normals.extend(facet[0]) | ||
|
|
||
| """ | ||
| if hasattr(model, 'indices') and model.indices: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you leaving this bit of code commented out for a future use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was inteded for future use, but the future turned out to be a pure OpenGl approach. I will clean it up.
printrun/gcodeplater.py
Outdated
| if answer == wx.OK: | ||
| self.DestroyLater() | ||
| return | ||
| # This works but no features are implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove these comments? I think the message works fine no need to load an empty window.
|
Hi @neofelis2X, I've played around a bit and found no show-stoppers. Great job!
I agree, it is something I've looked at before. But refactoring the settings system to use something less "homemade" like maybe ConfigParser, wasn't simple. Particularly regarding macros. I could not achieve a solution that wasn't a breaking change for users and/or required a lot of code and effort to keep backwards compatibility. |
|
Hi @neofelis2X, ... and I found two other (sorry):
Edit 2025-06-13: |
|
Hi @rockstorm101, good to hear and thank you very much. I saw only right now that you added some comments, i will look into them.
Ye I have no doubt that this would be difficult to achieve. Luckily there is no urgent need to change anything. Hello @DivingDuck, thanks I see we are making progress and even some older bugs get fixed. I think it's good to improve as much as possible, when it can be included easily in this PR. :) Regarding the [Fit to Frame] shortcut... For the platers it is not implemented. The Stl models do not even have bounding boxes yet, that's why platers just call [Reset view]. The problem with in the main view has probably todo with too big bounding boxes. i will test an example.
Hmm that is strange, seems to happen only on windows. In my pyglet2 branch it does not have any issues with loading. That could be a hint. Otherwise i'm at a loss here for now. |
Sorry, which Error? The plater never use perspective view, because this settings is not given to them. Do you mean the [Fit] Error? |
|
Hi @neofelis2X, For the Fit / Reset view problem: Shame on me regarding the stl and gcode plater issue, I looks like got confused during my tests. I can't reproduce it. You are right A little fun thing, this week I was searching in the math lib a function and remember a change you did in actors.py regarding Regarding the plater.py as stand alone app, it looks like something went wrong and crash or loop (? ) the app when it try to open the add model dialog a second time. Interestingly when I run it in visual studio in debug mode it sometimes is successful and sometimes not. In running from code w/o debugging it all times crash when I try to load a second stl file. It looks like there is a timing problem. I also try to run plater as binary file and it sometimes run and more often it hangs after I click the button Add Model for a additional stl file. |
|
We have a Fit to screen for the plater yay! It looks like there is a need for eliminating the first travel when we import a gcode model. This maybe causes some headache because of how do I identify what is the real model position and what is add-on code like travel or a purge line and so on. In the end this is where users responsibility come in place as I don't see a way how this can be solved automatically. The only "help" I can think of is giving the user the possibility of editing the gcode like we do this in the gcode view. |
Yes, I went in and added this functionality. It turned out to be much easier than I expected and I'm honestly very happy with the result.
Exactly, thats also what I found. Some Slicer configs add some extra extrusions to prime the nozzle and whatnot. The gcoder calculates the bounding box (minimum and maximum extensions) based on all extrusion moves. The only idea I have here is to not include the first layer into the bounding box. But that doesnt seem clean and is probably complicated. I'm better not touching the gcoder. I will implement a visible bounding box into the pyglet2 branch, maybe that is helpful.
Cool! I was not aware that tau is a thing in python. I will add it. |
|
I also had some time on windows to test the failing FileDialog.
Yes I tried it with Microsofts Python debugger and while stepping through it, it does not happen. When running normally, it hangs on the second time opening the dialog.
Maybe it has something to do with multithreaded COM calls. I found this wxWidgets issue on that topic: wxWidgets/wxWidgets#23578
|
|
Hi @rockstorm101 , @neofelis2X , Sorry that I had no time the last months. I think we should release this PR. I haven't a solution for the standalone jet but I guess that most of the users didn't use the standalone version of Plater - at least for Windows and macOS we do not publish a executable binary. We can open an issue to keep that problem in mind for future improvements. PS: Thanks for implementing TAU :) |
Hi,
it is long overdue that I push this work on the 3D view. It reorganises / moves / rewrites most of the code that handles the opengl canvas. It does not enable pyglet 2, everything is still based on pyglet 1.5. But it makes all the foundational changes necessary to use modern OpenGL (3.3+).
That incldues:
I know this are a lot of changes and all in one PR. It has taken me a while to untangle it all and I have changed bit by bit, going through it all. In my opinion this was necessary to get a more modern starting point to work with. Splitting it into smaller PRs would be a hindrance. On the other hand, the following PR for pyglet 2 should be more concise.
Note 1: When the diff says that there are 50.000 new lines of code then that is because I added a .gcode testfile.
Note 2: I already have a branch where everything runs on pyglet 2 and it's working really well. But still work in progress.
Fixes #746
Fixes #1035
Fixes #1301
I would be happy if you can test it on your respective operating system. On macOS it runs really well and I also tested it on windows and it looked good. Any input is welcome!