DX12 support#2
Conversation
wenzhang-unity
left a comment
wenzhang-unity
left a comment
There was a problem hiding this comment.
Overall looks pretty good. I have a couple of comments for you to look at 🙂
| .vs/ | ||
|
|
||
| # Files built by Visual Studio | ||
| *_i.c |
There was a problem hiding this comment.
Can you specify a directory (or directories) for VS-generated files? Specifying individual files could become a maintenance headache. Also, you have *.pdb in here, which you may not want ignored, since I see you have a .pdb file added to the repo.
There was a problem hiding this comment.
Created a .gitignore inside the ~Native directory to contain the VS rules
PDB deleted
|
|
||
| NativeRenderingPlugin.PixelFormat ToNativeRenderingPluginFormat(RSPixelFormat rsPixelFormat) | ||
| { | ||
| switch (rsPixelFormat) |
There was a problem hiding this comment.
This would be a good spot to use a switch expression
NativeRenderingPlugin.PixelFormat ToNativeRenderingPluginFormat(RSPixelFormat rsPixelFormat) =>
rsPixelFormat switch
{
RSPixelFormat.RS_FMT_BGRA8 => NativeRenderingPlugin.PixelFormat.BGRA8,
RSPixelFormat.RS_FMT_BGRX8 => NativeRenderingPlugin.PixelFormat.BGRX8,
// etc.
_ => NativeRenderingPlugin.PixelFormat.BGRA8 // alternatively, you may want to throw ArgumentOutOfRange here instead
}There was a problem hiding this comment.
Couldn't throw the exception when using the pattern matching syntax, decided to keep the old syntax but added exception throwing for both switches
There was a problem hiding this comment.
What do you mean? You can certainly throw exceptions in switch expressions.
There was a problem hiding this comment.
Using a throw statement with the nicer syntax gave me syntax errors, the compiler wouldn't allow ex:
NativeRenderingPlugin.PixelFormat ToNativeRenderingPluginFormat(RSPixelFormat rsPixelFormat) =>
rsPixelFormat switch
{
RSPixelFormat.RS_FMT_BGRA8 => NativeRenderingPlugin.PixelFormat.BGRA8,
RSPixelFormat.RS_FMT_BGRX8 => NativeRenderingPlugin.PixelFormat.BGRX8,
// etc.
_ => throw new ArgumentOutOfRange()
}
It's why I went with the old case X: return Y and default: throw ... syntax.
| m_streamHandle = 0; | ||
| } | ||
|
|
||
| Texture2D CreateTextureForDisguise(int width, int height, RSPixelFormat format) |
There was a problem hiding this comment.
Consider moving CreateTextureForDisguise and ResizeTextureForDisguise to a helper class. They can also be made static.
There was a problem hiding this comment.
Moved to DisguiseTextures.cs
wenzhang-unity
left a comment
wenzhang-unity
left a comment
There was a problem hiding this comment.
Just a minor thing. The rest looks good.
|
Feel free to merge it when you're ready. |
2 participants
No description provided.