Skip to content
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

added HBAO and DOF #5673

Closed
wants to merge 3 commits into from
Closed

added HBAO and DOF #5673

wants to merge 3 commits into from

Conversation

byumjin
Copy link
Contributor

@byumjin byumjin commented Jul 25, 2017

added HBAO and DOF with using PostprocessStage Composition (Gaussian Blur)

_2017_07_24_20_36_52_22
_2017_07_24_20_37_06_755
_2017_07_24_20_37_11_509
_2017_07_24_20_37_16_755
_2017_07_24_20_37_25_360

added HBAO and DOF
@pjcozzi
Copy link
Contributor

pjcozzi commented Jul 25, 2017

It might just be my eyes or the test scene, but it is hard for me to see the DOF effect in these screenshots? Is the blur blurry enough? And is the focus area small enough? Did you create a Sandcastle example with sliders for the various parameters?

@byumjin
Copy link
Contributor Author

byumjin commented Jul 25, 2017

Yes, it would be better to check the difference with Sandcastle because there are parameters for that
The Sandcastle's name is 'Depth of field'.

byumjin added 2 commits July 25, 2017 14:27
Added stepsize parameter at DOF
add Bloom(Glow) abd EdgeDetection(Toon)
Fixed a HBAO's artifact which draws unexpected lines

Glow -
http://localhost:8080/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=d5fd85d0eaec2a91fb9548ec8d8fbea4

Toon -
http://localhost:8080/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=87ac93ad6ac8df9c78894ba242416450
Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice overall. The one area that needs the most work is cleaning up the formatting of the shader code, but the code itself looks fine.

I'm looking forward to seeing Toon and Bloom in the main post-processing Sandcastle demo.

<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
<meta name="description" content="Change color of 3D models.">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the description. Also applies to the other Sandcastle demos.

@@ -0,0 +1,135 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the file to "Depth of Field" so it is clear to those not familiar with the acronym.

<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1, user-scalable=no">
<meta name="description" content="Change color of 3D models.">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the description.

<tr>
<td>Depth Of Field</td>
<td>
<input type="checkbox" data-bind="checked: DOFShow">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to depthOfFieldShow.

<tr>
<td>Focal Distance</td>
<td>
<input type="range" min="0.0" max="500.0" step="1" data-bind="value: FocalDistance, valueUpdate: 'input'">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use lower-case names instead - focalDistance, kernelSize, etc.

' isSpace = false; \n' +

//Sun position
' vec4 sunPos; \n' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name this sunPositionWC.

*/
this.lensFlare = PostProcessLibrary.lensFlare;

this.bloom = PostProcessLibrary.bloom;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bloom is missing the documentation.

* @private
*/
function PostProcessToonStage() {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove empty line.

*/
function PostProcessToonStage() {

this._ToonTexture = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to this._toonTexture. Also lower-case the other variables here.

};

return PostProcessToonStage;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline.

@lilleyse
Copy link
Contributor

@byumjin

I made a cleanup improvements to PostProcessAmbientOcclusionStage on my branch. Could you make similar changes to the other stages included in this PR? The diff is here: c5dccd4...93ef755

@lilleyse lilleyse mentioned this pull request Oct 19, 2017
12 tasks
@bagnell
Copy link
Contributor

bagnell commented Oct 23, 2017

I addressed all comments in another branch. I'll open a separate PR with the fixes.

@bagnell bagnell closed this Oct 23, 2017
@bagnell bagnell mentioned this pull request Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants