-
Notifications
You must be signed in to change notification settings - Fork 25
Add Texturing Chapter #136
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
Conversation
deccer
left a comment
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 like it a lot so far.
Just a few formatting/linking things and that one actual question regarding resource deletion after srv creation
| WRL::ComPtr<ID3D11ShaderResourceView> CreateTextureView(ID3D11Device* device, const std::wstring& pathToTexture) | ||
| { | ||
| FIBITMAP* image = nullptr; | ||
| //Open our file |
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.
Hmm code comments make no sense when the code is obvious already, that's why i want readable code
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've actually increased the comment a bit because "CreateFile" sounds counterintuitive to Opening a file (thanks winapi :) )
src/Cpp/1-getting-started/1-3-1-ImageLibrary/ImageLibraryApplication.cpp
Show resolved
Hide resolved
|
|
||
| size_t fileSize = GetFileSize(file, nullptr); | ||
| //Create our data buffer and read in the entire file | ||
| { |
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.
The scoping trick might not be obvious enough, I would suggest a separate private method for that
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'd personally rather add a comment explaining the scope, ideally I'd leave this entire texture loading at the two core functions it is currently.
Alternatively I can remove the scope entirely but this way we can keep memory usage across the function low and at best only keep FileSize * 2 amount of data in memory.
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.
If you explain the scope with a sentence or two, that should be OK then
| bool ImageLibraryApplication::Initialize() | ||
| bool TexturingApplication::Initialize() | ||
| { | ||
| // This section initializes GLFW and creates a Window |
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.
Hmm looks like we do have random comments which state the obvious already?????????
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.
removed it.
| - "ppShaderResourceViews" : The array of SRVs. | ||
|
|
||
| If we only need to set a single texture, this is as simple as calling: `PSSetShaderResources(0, 1, srv.Get())`. | ||
| If we call `PSSetShaderResources(2,2,srvs)` it will start at slot 2 and thus set slot 2 and 3, with the SRV's supplied. |
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.
All those slotisms, I wonder if it makes sense to insert a sentence here that those can be looked at in graphics debuggers as well, like RenderDoc and that we might have to massage/backreference/ofsorts to the debugging chapter somehow... do you know what i mean?
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.
Inconsistent spacing in the method params
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'll see what I can do with a renderdoc screenshot.
Fixed the spacings
deccer
left a comment
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.
LGTM
spec-chum
left a comment
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'm on mobile, but it all seems fine to me.
Manual texture loading with FreeImage will give us a good excuse to explain how to create textures, rather than let DirectXTex do it for us entirely.
Docs will follow later at some point