bsupnik / bricksmith

LDraw Modeler for OS X
BSD 2-Clause "Simplified" License
12 stars 6 forks source link

Bricksmith crashing because GL_VERTEXBUFFER, ... is not enabled #1

Open d029940 opened 5 years ago

d029940 commented 5 years ago

Im on MacOS Mojave and compiling with Xcode 10

I am getting a SIGART at bricksmith_bsupnik/trunk/Bricksmith/Source/LDraw/Support/LDrawGLRenderer.m, line 232: assert(glIsEnabled(GL_VERTEX_ARRAY));

It seems to be that GL_VERTEX_BUFFER was not enabled. This is also true for: assert(glIsEnabled(GL_NORMAL_ARRAY)); assert(glIsEnabled(GL_COLOR_ARRAY));

I put in LDrawGLView.m - internalInit - the following statements, just after // CGLEnable(CGLGetCurrentContext(), kCGLCEMPEngine);

glEnableClientState(GL_VERTEX_ARRAY);
glEnableClientState(GL_NORMAL_ARRAY);
glEnableClientState(GL_COLOR_ARRAY);

I am not sure what I am doing - I am not an OpenGL expert. But with these 3 statements, Bricksmith starts, but the windows managed by OpenGL are gray.

Are you using Xcode 9?

bsupnik commented 5 years ago

Hi,

I have Bricksmith running on Mojave and older OSes, but if it is compiled with Mojave against the 10.14 SDK, something goes wrong in view-clipping and I can't see anything. When compliing via X-code 8, the app works fine. I'll try to post a beta shortly.

On Mon, Jan 21, 2019 at 4:28 AM d029940 notifications@github.com wrote:

Im on MacOS Mojave and compiling with Xcode 10

I am getting a SIGART at bricksmith_bsupnik/trunk/Bricksmith/Source/LDraw/Support/LDrawGLRenderer.m, line 232: assert(glIsEnabled(GL_VERTEX_ARRAY));

It seems to be that GL_VERTEX_BUFFER was not enabled. This is also true for: assert(glIsEnabled(GL_NORMAL_ARRAY)); assert(glIsEnabled(GL_COLOR_ARRAY));

I put in LDrawGLView.m - internalInit - the following statements, just after // CGLEnable(CGLGetCurrentContext(), kCGLCEMPEngine);

glEnableClientState(GL_VERTEX_ARRAY); glEnableClientState(GL_NORMAL_ARRAY); glEnableClientState(GL_COLOR_ARRAY);

I am not sure what I am doing - I am not an OpenGL expert. But with these 3 statements, Bricksmith starts, but the windows managed by OpenGL are gray.

Are you using Xcode 9?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bsupnik/bricksmith/issues/1, or mute the thread https://github.com/notifications/unsubscribe-auth/ABqPH4g2PxBxmZoip0CXi3x8BzcrpUYBks5vFYghgaJpZM4aKaos .

ghost commented 4 years ago

The reason for the error is that the LDrawGLRenderer is nil at this point and thus, prepareOpenGL can not be called. It works if the initialization of the LDrawGLRenderer is moved before the creation of the context in LDrawGLView.m

Is this project still maintained? I could create a pull request with the modification.

bsupnik commented 4 years ago

@wayfaerer I have not seen this crash in current code. What's the GIT SHA where you see this and where your patch would apply? I'll look at whether the case is fixed in the latest code, then either merge that to master or take a pull.

ghost commented 4 years ago

@bsupnik I checked out the master, the sha is 9eaa764. The fix would move the lines 239 - 241 in LDrawGLView before line 211 where the NSOpenGLContext is initalized. The base sdk I'm using is 10.15.

d029940 commented 4 years ago

I just cloned the master. Opening the project file (Bricksmith.xcodeproj) in trunk, I am getting all sorts of warnings (SDK points to 10.12, 32-bit Architecture, ...), code signing error for ldsynth ...

And as far as I know, no one succeeded up to now using the sdk 10.15. It must be an older one because of the "layered Open-GL-View)

Am I doing something wrong? Am I correct?

ghost commented 4 years ago

Strange, I'm pretty sure I also have the project file from the trunk in the master. git log --pretty=format:'%h' -n 1 says: 9eaa764

I also got this code signing warning. It went away after I added "--deep" to "Other Code Signing Flags" for the Bricksmith Target.

d029940 commented 4 years ago

First of all, thank you. Adding "--deep" to "Other Code Signing Flags" helped. Also with moving the lines 239 - 241 in LDrawGLView before line 211 where the NSOpenGLContext is initalized, there is no runtime error anymore. But we are back at the start. On loading a model, the model is shortly flushed on the GLView and then disappears. (Building on Catalina, XCode 11.4.1 and most up to date sdk.

ghost commented 4 years ago

Yes, you will need one other fix. Add the Line [self setBackgroundColor:[NSColor clearColor]]; in initWithContentRect: of the class OverlayHelperWindow (line 68)

With this, the model should be visible, although it is streched vetically.

d029940 commented 4 years ago

Great. Thank you. That did the trick. The model is visible (and stretched). Is there a way to have a branch with all these litte modifications? So, that we have a change to clean the project file (no more 32 bit, ...)

I think, in order that bricksmith will survive, we need to convert to modern Obj-C and in the very end I think we need to move to Metal.

Two years ago I started with getting rid of retain/release for some modules, but was stopped by the OpenGL problem wit Catalina. I also had a look into Metal. But beyond some easy tutorials, it is really very difficult - and at least if you have no knowledge about OpenGL and only limited knowledge about 3D.

ghost commented 4 years ago

Basically that should not be a problem. @bsupnik did you already have the time to look into the latest code? What branch is this on?

@d029940 I thought exactly the same, bringing the project to ARC and eventually to Metal will be necessary for Bricksmith to survive. Unfortunately, I also don't know much about OpenGL and Metal. There's a Talk at WWDC for such a migration by the way. (https://developer.apple.com/videos/play/wwdc2019/611/)

It would really be a shame if Bricksmith dies, it's a very good editor for LDraw. I think I would have some time to spare for this project, but I think it's a lot of work to be done overall.

bsupnik commented 4 years ago

Hi Y'all,

I'm running on e5cfed1393f2f6cad69f0e7b6e4882b5f33956d9. Note that you need an old 10.12 SDK to build this.

This is the only way I have found to have zoom work on Mojave and Catalina. In older GL-based apps, the content view doesn't have its own layer, and therefore when it "zooms" the pixel density doesn't change. This changed recently - it's as if "has its own layer" is always forced on, and the result is that BrickSmith's zooming strategy completely breaks down. In the long term, we clearly need new zoom code that doesn't depend on the old GL-borrows-a-layer trick, it was just beyond the scope of what I had time for to make that mod.

This branch compiles, links, and runs correctly when built on Mojave; when built on Catalina, you have to use a linker flag to hack around a missing symbol and then the zoom code still ends up buggy - zooming in shows pixelation.

So...I think this is my thinking:

AppKit is very mature and complex and internal new features do things like change callback orders in ways that break old BrickSmith code. So UI is the tech debt I would like to see paid off first. It's also the thing I'm least qualified to deal with, as I'm not an AppKit/ObjC dev by trade.

I am less worried about Apple EOLing OpenGL - there's a whole ecosystem of pro CAD and authoring apps that content creators use on OS X that depend on it, so if I had to put my money down, it would be on Apple leaving it as is and simply never updating it.

ghost commented 4 years ago

This all sounds very reasonable! I checked out e5cfed1 and compiled it with Base SDK 10.15 (deleting derived data before) and it compiled with the „—deep“ code signing flag. I changed the Architecture to Standard to drop 32 bit code. Also, I still need to set the background to clearColor in the OverlayHelperWindow. The assertion error with the Open GL Flags is gone in e5cfed1.

This is what it looks now:

image

There is no distortion in the 3D View anymore, but still in the front and side views.

I only see pixels with zooming when I go to a very high zoom level, like 500%:

image

Is this what you are refering to?

Unfortunately I have no experience with OpenGL and not much with AppKit, I am quite experienced with IOS development and still familiar with Objective C, might need some time to get back into manual reference counting, though…

What I did not run into are the linker flag and the issue with the init order, but I also didn’t test much.

What about also setting the deployment target to something like at least 10.12 and go after the deprecations? Should also get rid of some old ballast and would be something I could have a look at for a start.

bsupnik commented 4 years ago

@wayfaerer this is exactly what I expect -- distortion in the views and you can see the GL pixels when you move in.

It is possible to "fix" this by changing the rect we use, but doing so breaks Mojave builds where the old SDK is enough to get the old CALayer behavior. I haven't bothered to check in a "Catalina" version because the second problem is still broken.

In other words, we've lost the ability of the pixel density (in terms of bricks) to change with zoom.

ANYWAY...if you'd like to make a topic branch off e5cfed139 to check in some Catalina fixes (no 32-bits, modern SDK, any other fixes), go for it and I'll take a pull request.

In terms of code cleanup, I think my main concern is:

I could probably commit to not doing that refactoring any time soon -- the truth is, I've had about zero time to work on BrickSmith and it'd be wasteful to ask you to wait for cleanup that I might do if I can't say when I'm going to get to it.

But in the end, the Catalina view issue is probably the most important one - once that's fixed, wec can unify and everything becomes easier again.

ghost commented 4 years ago

It surly makes total sense to start with the Open-GL issues! Unfortunately this looks like beeing far out of reach for me to give it a try.