Hi Rui, I have now begun a review of your changes to ViewDependentShadowMap, I plan to do it via several passes as the original code was pretty complicated, and the new code has more classes and functionality making it even more complicated together this makes for a tough submission to review and get right, especially as it's quite a few months since I worked on the original code.
>From my first pass my first thoughts are: In the future I'm inclined to evolve osgShadow towards being based on ViewDepedentShadowMap as it's base rather than the current collection of curious and not so useful shadow implementations. This evolution will really depend upon how ViewDependentShadowMap pans out, but for now that's the direction I'm optimitic about heading. This will be something for OSG-3.4 rather than OSG-3.2 though, so for now focus has to be on getting ViewDependentShadowMap the best it can be - which means something based upon your latest revisions. ViewDepdentShadowMapUtils is really just a collection of Utils, that just happen to only be used by ViewDependentShadowMap right now. So.. I'm inclined to just simplify the header and .cpp name. to Utils or something else as generic. The name change of ShadowSettings::setMaximumShadowMapDistance() to ShadowSettings::setMaximumShadowMapNearFarDistance() suggests a change in this parameters role, but I'm not yet clear how that could involved Near and Far Distance as both Near and Far can't ever be the same distance, this is also a change in the API that really has to be justified, and in this instance if the name really has to change the I can't see any reason to have Near in the name as it's misleading. The removal of ShadowSettings::setShadowMapProjectionHint() and setMultipleShadowMapHint(), replacing them with be bit mask setShadowMapTechniqueHints() both breaks the API and is limiting for no gain of functionality. While both the ShadowMapProjectionHint and MultipleShadowMapHint currently only have two options each I do not envisage this to be the case going forward. There are lots of different ways to multiple shadow maps - parallel split is only one, and even the projection side has papers that use custom projections, given this it's a backwards step to force a bit mask that has no capability of handling the introduction of new enum's and back end functionality that they map to. My suggestion would be to revert this to the original method names. Implementation wise I'm still digesting the changes. I'll let you comment on the above before I dive back in for another pass. Cheers, Robert. _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
