search

KhronosGroup / glTF-Sample-Assets

To store all models and other assets related to glTF
255 stars 30 forks source link

Add DispersionTest model #94

Closed emackey closed 4 months ago

emackey commented 5 months ago

screenshot-large

DRx3D commented 5 months ago

According to Submitting Models this PR needs 2 approvals. This PR will be merged when that happens.

emackey commented 5 months ago

this PR needs 2 approvals

In general I think that's a poor policy decision for a repo with so few active reviewers. We would need a vibrant, responsive team of active reviewers to make this work.

MiiBond commented 5 months ago

Here's the model rendered in Adobe Substance 3D Stager. The IBL is different than the one used in Babylon, of couse, but they're both black and white. Obviously, they result is very different than with the rasterizer approximation but, then again, refraction is very different too so I don't think we can expect dispersion to match that closely.

DispersionExample

DRx3D commented 4 months ago

@emackey : The review policy changed to make things easier. Models now need 1 (positive) review in order to merge. The intent is to make sure the model does what it says it does. Can you get someone to review this?

emackey commented 4 months ago

Hi Leonard, this model has been reviewed in the PBR TSG calls several times now, and earlier versions of this model underwent changes based on those reviews to change it into what was submitted on Jan 5.

I don't see much point in further waiting for a GitHub user to comment on this. It's been almost 2 months and no one has stepped forward to click on any GitHub review button here. The various experts on our call who know the PBR dispersion math best have kindly given us screenshots and feedback that has all been addressed.

Do you mind if I hit the merge button myself? I'd like to get this merged before Wednesday's vote.

emackey commented 4 months ago

Thanks @lexaknyazev!