GPUOpen-Effects / TressFX

DirectX 12 and Vulkan libraries that provide convenient access to realistically rendered and simulated hair and fur
MIT License
795 stars 131 forks source link

Possible typos #2

Closed Mixaill closed 8 years ago

Mixaill commented 8 years ago

1) /amd_tressfx_viewer/src/TFXFileIO.cpp

272             else if ( token.find(L"Ex1") != std::string::npos ) 
273             { 
274                 Ex2 =  (float)_wtof(sTokens[1].c_str()); 
275             } 

Line 272 : should be Ex2 instead of Ex1 ?

2) /amd_tressfx_viewer/src/main.cpp

402    g_RenderHUD.m_GUI.AddSlider( IDC_OPACITY, iX, iY += AMD::HUD::iElementDelta, AMD::HUD::iElementWidth, AMD::HUD::iElementHeight, 
403         0, 100, (int)(100 * (g_hairAlpha.min - g_hairAlpha.min) / (g_hairAlpha.max - g_hairAlpha.min)) ); 
404 

Line 403: should be (int)(100 * (g_hairAlpha.max - g_hairAlpha.min) / (g_hairAlpha.max - g_hairAlpha.min) ?

deepbluev7 commented 8 years ago

Also in /amd_tressfx/src/TressFX.cpp:

211    //
212    // TressFX_Resize
213    //

Line 212 should be TressFX_Release

232    //--------------------------------------------------------------------------------------
233    //
234    // TressFX_Resize
235    //
236    // Releases any memory allocated by the library
237    //
238    //--------------------------------------------------------------------------------------

This comment is an exact copy of the one before and doesn't match the function TressFX_GenerateTransforms

ExtReMLapin commented 8 years ago

Why don't you make a pull request ?

deepbluev7 commented 8 years ago

I don't know, what would be the correct comment in the second case, so I leave both changes up to someone, who actually knows, how to document the TressFX_GenerateTransforms function. @Mixaill s changes seem to be correct though and they could be a pull request, that could still be dismissed, if they are wrong.

jstewart-amd commented 8 years ago

Note that we (the AMD employees working on TressFX) are not currently working directly in GitHub. We may be eventually, but for now, for these types of small, isolated changes, it is easier for me to manually incorporate the suggested fixes in our internal code, and then they will be part of the next release we push out to the public GitHub repo.

Thanks for the feedback. It is very useful.

jstewart-amd commented 8 years ago

Regarding my previous comment, "but for now, for these types of small, isolated changes, it is easier for me to manually incorporate the suggested fixes in our internal code", never mind. We now welcome pull requests. See the contribution guidelines in CONTRIBUTING.md.

jstewart-amd commented 8 years ago

Regarding the question from @Mixaill about AddSlider, that was not a bug, per se. But the way we were initializing the sliders was kind of strange and confusing. I cleaned that up in 475ed37ab459af91ac3f04670319ce7c978ec542.

The "should be Ex2 instead of Ex1" issue was a bug. I fixed it in 526af449cd117124167505d3edc5c51c77bb4ade.

Thanks, @Mixaill.

jstewart-amd commented 8 years ago

And I did a quick cleanup pass on the function header comments in TressFX.cpp to fix the copy-paste errors @MonokelPinguin found. See 09753d54f40b72492ae5d057fa536fe5983197dc.