Addition of Chapter 3 Part 1: Model Loading #76
Conversation
There was a problem hiding this comment.
Really nice work!
It's great to receive such substantial contributions!
Thanks a ton!
This is looking quite good for a start.
I've commented on bunch of stuff that can be improved with this chapter, mostly about adding more comments describing what is going on and why.
There are also some things we can document here about how to use OpenTK with math types, and also how to be efficient with memory and avoiding copying it unnecessarily. Those code parts I could help with if you want me to. :)
Common/Model.cs
Outdated
| //Create a new importer | ||
| AssimpContext importer = new AssimpContext(); | ||
|
|
||
| //This is how we add a logging callback |
There was a problem hiding this comment.
This comes directly from the Assimp-Net Wiki and from what I understand, it attaches a function to the importer so that during the import you can see the steps that are happening and they will be written to the console. The documentation for AssimpNet is rather lacking (I couldn't get their chm file to work. I am adding some comments to explain it better, since looking more into it, I think it is a very useful debugging tool.
There was a problem hiding this comment.
Are you interested in expanding on this comment to explain it a bit more? Should be the last thing before I can merge it.
There was a problem hiding this comment.
I made some changes to the comments which hopefully clarify this better.
|
I just made a second commit with these changes. I would especially appreciate help working with implementing |
|
I've added a new commit to this PR where I refactor a few things. Please take a look and tell me what you think 🙂 Seems like you've got this thing with commits and PRs down 😎, wouldn't know you aren't used to PRs if you didn't tell me. |
…d CursorGrabbed to CusorState. Updated assimp matrix conversion comment to reflect column-major state.
|
I did some cleanup and tried to improve the comments throughout. Thanks for catching the inverse transformation matrix needed for the normals. I actually realized this later and was pleasantly surprised to see it had already been fixed. In my latest commit, I specifically mentioned the comment about the Assimp matrix being column-major to make sure I had that right. Let me know if you see anything else we should change, or go ahead and make the changes. Thanks! |
…() method. Made a few minor changes to comments to help the code be more in line with the online tutorial.
|
OpenTK Chapter 3_07-09-22.docx I updated the documentation to match our changes and made a small commit to go with it. I think we are almost there. |
Common/Model.cs
Outdated
| public void Draw() | ||
| { | ||
| for (int i = 0; i < meshes.Count; i++) | ||
| meshes[i].Draw(); | ||
| } |
There was a problem hiding this comment.
I would actually like to see this logic moved into the chapter code itself. This type of abstraction using objects with a Draw method isn't an abstraction that works in the long term for graphics. It becomes really inefficient, so I think that we should avoid making the suggestion that this is a good way to structure rendering code.
There was a problem hiding this comment.
This means both the mesh and the model drawing code.
And I know the other abstractions like texture and shaders do this type of things too with their Use methods, but I'm not happy with those either.
|
Also is the |
|
@adamsmith1990 What is the status of this? Should I do the last pieces to get this merged or do you have time to do them? |
…n per NogginBops request.
|
I've just gotten over COVID, so thanks for you patience. I think the commit I just made fits what you were looking for. This way there is a bit more control over how things are drawn and the meshes are now just containers for VAOs and models are just containers for meshes from a loaded model. |
|
|
||
| GL.Enable(EnableCap.DepthTest); | ||
|
|
||
| // Backpack by Berk Gedik: https://sketchfab.com/3d-models/survival-guitar-backpack-low-poly-799f8c4511f84fab8c3f12887f7e6b36 |
There was a problem hiding this comment.
According to the license the model is under you need to not only give attribution to the creator but also link the license.
You also need to specify if any changes where made to the work.
I would suggest creating a license.txt or attribution.txt in Resources/Backpack where you put the sketchfab link, the license link and specify if any changes where made.
|
Nice addition! 
Regarding the comment "I had to adjust the texture loading significantly to get it to work properly". I have the same C++ program from LearnOpenGL compiled, so I can debug to understand more deeply what remains to be done here. |
|
Ok I've got a draft version working which mimicks learnopengl (loading of OBJ model including the various textures). I might propose a dedicated PR soon if it's OK. 
|
This is my attempt at beginning a Chapter 3 based on https://learnopengl.com
This covers loading a model using AssimpNet and rendering it. This could be continued with another chapter that applies the lighting techniques used in previous chapters.
I had to adjust the texture loading significantly to get it to work properly. For some reason when I tried to load the .obj that the original author supplied (https://learnopengl.com/data/models/backpack.zip), it got stuck in the model loading loop and crashed after using up all of my memory. So instead I just downloaded the original fbx and loaded the texture directly into a Texture object and used a simple shader to apply it.
I adjusted the tutorial from https://learnopengl.com and have attached it to this pull request. I did my best, but I would love for this to be reviewed and standardized to match the rest of the chapters. I hope you like what I have put together!
I did just read through your contributing guidelines, and in the future I will use Early Pull requests.
OpenTK Chapter 3.docx