Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Sat, 25 Apr 2020 17:07:21 GMT, Kevin Rushforth wrote: >> @kevinrushforth >> Member >> kevinrushforth commented Apr 18, 2020 >> >> I think most of those are good suggestions going forward. As for the >> performance drop, the only place we've seen it so >> far is on graphics accelerators that are a few years old by now. >> So 50% drop on a 2015 macbook pro is OK ? Do we have numbers on recent >> macbook pros ? > > If this were an even remotely representative use case, then no, the > performance hit would not be OK. The test was > designed as an artificial "worst-case" stress test: a single mesh with a > large number of very large (window-sized) > quads stacked on top of each other. Any real-world use case won't do this. > We should make sure that we aren't seeing > any significant performance drop when rendering spheres (at a couple > different tessellation levels) or boxes. Results with NVIDIA Quadro P400: Without the fix, 1000 quads, average FPS ~7.4 With the fix, 1000 quads, average FPS ~6.1 - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
> > Will there also be any performance drop in case you just use the default > parameters for the lighting? That's what we're testing. The default, which corresponds to the current lighting, should not need > any additional computations > and thus no performance drop. But there is no way to know if you're using the default values unless you check for it, and that already is a cost. The only way to guarantee that there is no performance drop is to add a boolean that enables/disables these changes, and that will require generating double the shaders - for the "on" case and for the "off" case. We are trying to avoid this. These changes should be negligible according to "current knowledge", but we have to test. For some reason we saw a drop in one of the hardware configurations that was tested. On Sat, Apr 25, 2020 at 8:33 PM Michael Paus wrote: > Am 25.04.20 um 19:09 schrieb Kevin Rushforth: > > On Fri, 24 Apr 2020 05:06:56 GMT, Phil Race wrote: > > > >>> Here is a slightly modified test program. It fixes a compilation error > in the previous, and also adds a system property > >>> to set the number of quads: > >>> It creates 200 quads by default. If you need to increase this or > decrease it to get something in the ~ 10 fps range you > >>> can do that with `-DnumQuads=`. > >>> [pointlighttest.zip]( > https://github.com/openjdk/jfx/files/4526179/pointlighttest.zip) > >> @kevinrushforth > >> Member > >> kevinrushforth commented Apr 18, 2020 > >> > >> I think most of those are good suggestions going forward. As for the > performance drop, the only place we've seen it so > >> far is on graphics accelerators that are a few years old by now. > >> So 50% drop on a 2015 macbook pro is OK ? Do we have numbers on recent > macbook pros ? > > If this were an even remotely representative use case, then no, the > performance hit would not be OK. The test was > > designed as an artificial "worst-case" stress test: a single mesh with a > large number of very large (window-sized) > > quads stacked on top of each other. Any real-world use case won't do > this. > > > > We should make sure that we aren't seeing any significant performance > drop when rendering spheres (at a couple > > different tessellation levels) or boxes. > > > > - > > > > PR: https://git.openjdk.java.net/jfx/pull/43 > > Will there also be any performance drop in case you just use the default > parameters for the lighting? > The default, which corresponds to the current lighting, should not need > any additional computations > and thus no performance drop. > >
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
Am 25.04.20 um 19:09 schrieb Kevin Rushforth: On Fri, 24 Apr 2020 05:06:56 GMT, Phil Race wrote: Here is a slightly modified test program. It fixes a compilation error in the previous, and also adds a system property to set the number of quads: It creates 200 quads by default. If you need to increase this or decrease it to get something in the ~ 10 fps range you can do that with `-DnumQuads=`. [pointlighttest.zip](https://github.com/openjdk/jfx/files/4526179/pointlighttest.zip) @kevinrushforth Member kevinrushforth commented Apr 18, 2020 I think most of those are good suggestions going forward. As for the performance drop, the only place we've seen it so far is on graphics accelerators that are a few years old by now. So 50% drop on a 2015 macbook pro is OK ? Do we have numbers on recent macbook pros ? If this were an even remotely representative use case, then no, the performance hit would not be OK. The test was designed as an artificial "worst-case" stress test: a single mesh with a large number of very large (window-sized) quads stacked on top of each other. Any real-world use case won't do this. We should make sure that we aren't seeing any significant performance drop when rendering spheres (at a couple different tessellation levels) or boxes. - PR: https://git.openjdk.java.net/jfx/pull/43 Will there also be any performance drop in case you just use the default parameters for the lighting? The default, which corresponds to the current lighting, should not need any additional computations and thus no performance drop.
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Fri, 24 Apr 2020 05:06:56 GMT, Phil Race wrote: >> Here is a slightly modified test program. It fixes a compilation error in >> the previous, and also adds a system property >> to set the number of quads: >> It creates 200 quads by default. If you need to increase this or decrease it >> to get something in the ~ 10 fps range you >> can do that with `-DnumQuads=`. >> [pointlighttest.zip](https://github.com/openjdk/jfx/files/4526179/pointlighttest.zip) > > @kevinrushforth > Member > kevinrushforth commented Apr 18, 2020 > > I think most of those are good suggestions going forward. As for the > performance drop, the only place we've seen it so > far is on graphics accelerators that are a few years old by now. > So 50% drop on a 2015 macbook pro is OK ? Do we have numbers on recent > macbook pros ? If this were an even remotely representative use case, then no, the performance hit would not be OK. The test was designed as an artificial "worst-case" stress test: a single mesh with a large number of very large (window-sized) quads stacked on top of each other. Any real-world use case won't do this. We should make sure that we aren't seeing any significant performance drop when rendering spheres (at a couple different tessellation levels) or boxes. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Fri, 24 Apr 2020 01:50:11 GMT, Kevin Rushforth wrote: >> I think most of those are good suggestions going forward. As for the >> performance drop, the only place we've seen it so >> far is on graphics accelerators that are a few years old by now. Integrated >> graphics chipsets (such as Intel HD) either >> old or new seem largely unaffected by the shader changes. What we are >> missing is performance metrics from newer >> graphics accelerators on Mac and Windows. Even with the performance drop on >> older graphics devices, I'm leaning >> towards not having the shaders to be shaders to be doubled, since this is an >> artificial stress test with huge quads. If >> we could get performance data from a couple more recent graphics >> accelerators that would be best. > > Here is a slightly modified test program. It fixes a compilation error in the > previous, and also adds a system property > to set the number of quads: > It creates 200 quads by default. If you need to increase this or decrease it > to get something in the ~ 10 fps range you > can do that with `-DnumQuads=`. > [pointlighttest.zip](https://github.com/openjdk/jfx/files/4526179/pointlighttest.zip) @kevinrushforth Member kevinrushforth commented Apr 18, 2020 I think most of those are good suggestions going forward. As for the performance drop, the only place we've seen it so far is on graphics accelerators that are a few years old by now. So 50% drop on a 2015 macbook pro is OK ? Do we have numbers on recent macbook pros ? - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Sat, 18 Apr 2020 15:45:32 GMT, Kevin Rushforth wrote: >> I discussed this with a graphics engineer. He said that a couple of branches >> do not have any real performance impact >> even on modern mobile devices, and that, e.g., on iOS 7 using half floats >> instead of floats was improving shader >> execution dramatically. Desktops with NVIDIA or AMD and even Intel modern >> cards can process dozens of branches with no >> significant performance degradation. He suggested actually to have all the >> light types in a single shader file >> (looking ahead here). He also suggested not to permute on shaders based on >> the number of lights and just pass in a >> uniform for that number and loop over it. The permutations on the bump, >> specular and self illuminations components are >> correct (not sure we are not doing that for the diffuse component). If we >> add later shadows, which is not on my near >> to-do list, then we should permute there. It also depends on our target >> hardware. If we take into account hardware >> from, say, 2005 then maybe branching will cause significant performance >> loss, but that hinders our ability to increase >> performance for newer hardware. What is the policy here? I have a Win10 >> laptop with a GeForce 610M that I will test >> this weekend to see if the mobile NVidia cards have some issue. > > I think most of those are good suggestions going forward. As for the > performance drop, the only place we've seen it so > far is on graphics accelerators that are a few years old by now. Integrated > graphics chipsets (such as Intel HD) either > old or new seem largely unaffected by the shader changes. What we are missing > is performance metrics from newer > graphics accelerators on Mac and Windows. Even with the performance drop on > older graphics devices, I'm leaning > towards not having the shaders to be shaders to be doubled, since this is an > artificial stress test with huge quads. If > we could get performance data from a couple more recent graphics accelerators > that would be best. Here is a slightly modified test program. It fixes a compilation error in the previous, and also adds a system property to set the number of quads: It creates 200 quads by default. If you need to increase this or decrease it to get something in the ~ 10 fps range you can do that with `-DnumQuads=`. [pointlighttest.zip](https://github.com/openjdk/jfx/files/4526179/pointlighttest.zip) - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Fri, 17 Apr 2020 16:02:12 GMT, Nir Lisker wrote: >> Conclusion: The new shaders that support attenuation don't seem to have much >> of a performance impact on machines with >> an Intel HD, but on systems with a graphics accelerator, it is a significant >> slowdown. >> So we are left with the two choices of doubling the number of shaders (that >> is, a set of shaders with attenuation and a >> set without) or living with the performance hit (which will only be a >> problem on machines with a dedicated graphics >> accelerator for highly fill-limited scenes). The only way we can justify a >> 2x drop in performance is if we are fairly >> certain that this is a corner case, and thus unlikely to hit real >> applications. If we do end up deciding to replicate >> the shaders, I don't think it is all that much work. I'm more worried about >> how well it would scale to subsequent >> improvements, although we could easily decide that for, say, spotlights >> attenuation is so common that you wouldn't >> create a version that doesn't do that. In the D3D HLSL shaders, ifdefs are >> used, so the work would be to restore the >> original code and add the new code under an ifdef. Then double the number of >> lines of gradle (at that point, I'd do it >> in a for-each loop), then modify the logic that loads the shaders to pick >> the right one. For GLSL, the different parts >> of the shader are in different files, so it's a matter of creating new >> versions of each of the three lighting shaders >> that handle attenuation and choosing the right one at runtime. > > I discussed this with a graphics engineer. He said that a couple of branches > do not have any real performance impact > even on modern mobile devices, and that, e.g., on iOS 7 using half floats > instead of floats was improving shader > execution dramatically. Desktops with NVIDIA or AMD and even Intel modern > cards can process dozens of branches with no > significant performance degradation. He suggested actually to have all the > light types in a single shader file > (looking ahead here). He also suggested not to permute on shaders based on > the number of lights and just pass in a > uniform for that number and loop over it. The permutations on the bump, > specular and self illuminations components are > correct (not sure we are not doing that for the diffuse component). If we add > later shadows, which is not on my near > to-do list, then we should permute there. It also depends on our target > hardware. If we take into account hardware > from, say, 2005 then maybe branching will cause significant performance loss, > but that hinders our ability to increase > performance for newer hardware. What is the policy here? I have a Win10 > laptop with a GeForce 610M that I will test > this weekend to see if the mobile NVidia cards have some issue. I think most of those are good suggestions going forward. As for the performance drop, the only place we've seen it so far is on graphics accelerators that are a few years old by now. Integrated graphics chipsets (such as Intel HD) either old or new seem largely unaffected by the shader changes. What we are missing is performance metrics from newer graphics accelerators on Mac and Windows. Even with the performance drop on older graphics devices, I'm leaning towards not having the shaders to be shaders to be doubled, since this is an artificial stress test with huge quads. If we could get performance data from a couple more recent graphics accelerators that would be best. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Wed, 15 Apr 2020 20:59:40 GMT, Kevin Rushforth wrote: >> Here are the results on Phil's machine, which is a Mac Book Pro with a >> graphics accelerator (Nvidia, I think). >> >> Without the patch: >> 2000 quads average 8.805 fps >> >> With the patch: >> 2000 quads average 4.719 fps >> >> Almost a 2x performance hit. > > Conclusion: The new shaders that support attenuation don't seem to have much > of a performance impact on machines with > an Intel HD, but on systems with a graphics accelerator, it is a significant > slowdown. > So we are left with the two choices of doubling the number of shaders (that > is, a set of shaders with attenuation and a > set without) or living with the performance hit (which will only be a problem > on machines with a dedicated graphics > accelerator for highly fill-limited scenes). The only way we can justify a 2x > drop in performance is if we are fairly > certain that this is a corner case, and thus unlikely to hit real > applications. If we do end up deciding to replicate > the shaders, I don't think it is all that much work. I'm more worried about > how well it would scale to subsequent > improvements, although we could easily decide that for, say, spotlights > attenuation is so common that you wouldn't > create a version that doesn't do that. In the D3D HLSL shaders, ifdefs are > used, so the work would be to restore the > original code and add the new code under an ifdef. Then double the number of > lines of gradle (at that point, I'd do it > in a for-each loop), then modify the logic that loads the shaders to pick the > right one. For GLSL, the different parts > of the shader are in different files, so it's a matter of creating new > versions of each of the three lighting shaders > that handle attenuation and choosing the right one at runtime. I discussed this with a graphics engineer. He said that a couple of branches do not have any real performance impact even on modern mobile devices, and that, e.g., on iOS 7 using half floats instead of floats was improving shader execution dramatically. Desktops with NVIDIA or AMD and even Intel modern cards can process dozens of branches with no significant performance degradation. He suggested actually to have all the light types in a single shader file (looking ahead here). He also suggested not to permute on shaders based on the number of lights and just pass in a uniform for that number and loop over it. The permutations on the bump, specular and self illuminations components are correct (not sure we are not doing that for the diffuse component). If we add later shadows, which is not on my near to-do list, then we should permute there. It also depends on our target hardware. If we take into account hardware from, say, 2005 then maybe branching will cause significant performance loss, but that hinders our ability to increase performance for newer hardware. What is the policy here? I have a Win10 laptop with a GeForce 610M that I will test this weekend to see if the mobile NVidia cards have some issue. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Tue, 7 Apr 2020 23:03:21 GMT, Kevin Rushforth wrote: >> @arapte Can you please test the performance changes too? > > I think @arapte has a similar MacBookPro model to mine. > > I think @prrace might be able to test it (I'll sync with him offline). Here are the results on Phil's machine, which is a Mac Book Pro with a graphics accelerator (Nvidia, I think). Without the patch: 2000 quads average 8.805 fps With the patch: 2000 quads average 4.719 fps Almost a 2x performance hit. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Wed, 15 Apr 2020 20:59:29 GMT, Kevin Rushforth wrote: >> I think @arapte has a similar MacBookPro model to mine. >> >> I think @prrace might be able to test it (I'll sync with him offline). > > Here are the results on Phil's machine, which is a Mac Book Pro with a > graphics accelerator (Nvidia, I think). > > Without the patch: > 2000 quads average 8.805 fps > > With the patch: > 2000 quads average 4.719 fps > > Almost a 2x performance hit. Conclusion: The new shaders that support attenuation don't seem to have much of a performance impact on machines with an Intel HD, but on systems with a graphics accelerator, it is a significant slowdown. So we are left with the two choices of doubling the number of shaders (that is, a set of shaders with attenuation and a set without) or living with the performance hit (which will only be a problem on machines with a dedicated graphics accelerator for highly fill-limited scenes). The only way we can justify a 2x drop in performance is if we are fairly certain that this is a corner case, and thus unlikely to hit real applications. If we do end up deciding to replicate the shaders, I don't think it is all that much work. I'm more worried about how well it would scale to subsequent improvements, although we could easily decide that for, say, spotlights attenuation is so common that you wouldn't create a version that doesn't do that. In the D3D HLSL shaders, ifdefs are used, so the work would be to restore the original code and add the new code under an ifdef. Then double the number of lines of gradle (at that point, I'd do it in a for-each loop), then modify the logic that loads the shaders to pick the right one. For GLSL, the different parts of the shader are in different files, so it's a matter of creating new versions of each of the three lighting shaders that handle attenuation and choosing the right one at runtime. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Thu, 2 Apr 2020 12:59:11 GMT, Nir Lisker wrote: >> I have added few comments, but have not run tests and sample yet. > > @arapte Can you please test the performance changes too? I think @arapte has a similar MacBookPro model to mine. I think @prrace might be able to test it (I'll sync with him offline). - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Fri, 3 Jan 2020 09:26:43 GMT, Ambarish Rapte wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Attenuation and range changed internally to floats from doubles > > I have added few comments, but have not run tests and sample yet. @arapte Can you please test the performance changes too? - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Tue, 17 Mar 2020 02:50:28 GMT, Nir Lisker wrote: >> Updated test case: >> [attenTest2.zip](https://github.com/openjdk/jfx/files/4332937/attenTest2.zip) > > On Win 10 with an AMD RX 470 4GB I get the following: > > Without the patch: > 200 quads average 113 fps > 5000 quads average 11.5 fps > > With the patch: > 200 quads average 106 fps fps > 5000 quads average 8.5 fps > > Will test on Ubuntu later. On Ubuntu 18 with an AMD RX 470 4GB I get the following: Without the patch: 200 quads average 107 fps 5000 quads average 11.5 fps With the patch: 200 quads average 107 fps fps 5000 quads average 11 fps - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Sat, 14 Mar 2020 15:31:18 GMT, Kevin Rushforth wrote: >> I'll attach the above modified testcase that I ran. I ran it on a relatively >> new Windows 10 laptop and a rather ancient >> MacBook Pro. I had to drastically reduce the number of quads on the Mac, but >> the results are similar: no significant >> difference between the current code and the proposed patch for point lights >> (without attenuation). I'd like to see >> results on a recent machine with a graphics accelerator (either NVIDIA or >> AMD/ATI) to see if the new shader hurts >> performance there, but I suspect it will be fine. > > Updated test case: > [attenTest2.zip](https://github.com/openjdk/jfx/files/4332937/attenTest2.zip) On Win 10 with an AMD RX 470 4GB I get the following: Without the patch: 200 quads average 113 fps 5000 quads average 11.5 fps With the patch: 200 quads average 106 fps fps 5000 quads average 8.5 fps Will test on Ubuntu later. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Wed, 11 Mar 2020 21:35:55 GMT, Kevin Rushforth wrote: >> I'll take a look. My quick thought is that we need some sort of test with a >> reasonable number of large boxes (so that >> it is fill-limited). If there isn't such a case, and the 3D rendering is >> always node-limited, then the shader >> performance doesn't really matter all that much. I suspect we should be able >> to find a case where it does, but we'll >> see. > > I did some limited testing today with a modification to the test program you > attached to create a MeshView with 200 > large quads (400 triangles) in a single node. This will eliminate the node > overhead. I can confirm that it is fill rate > limited, because when I send the exact same amount of data, but make the > triangles small, the frame rate goes up as > expected. It sill looks like it isn't shader limited, though, at least on my > Windows 10 machine, which has an Intel > UHD Graphics 630. More testing is needed on other platforms. I'll share the > mods to the test program when I have time, > but it's basically just creating a set of quads on top of each other by > reusing the same 4 points in each pair of faces. I'll attach the above modified testcase that I ran. I ran it on a relatively new Windows 10 laptop and a rather ancient MacBook Pro. I had to drastically reduce the number of quads on the Mac, but the results are similar: no significant difference between the current code and the proposed patch for point lights (without attenuation). I'd like to see results on a recent machine with a graphics accelerator (either NVIDIA or AMD/ATI) to see if the new shader hurts performance there, but I suspect it will be fine. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Sat, 14 Mar 2020 15:29:59 GMT, Kevin Rushforth wrote: >> I did some limited testing today with a modification to the test program you >> attached to create a MeshView with 200 >> large quads (400 triangles) in a single node. This will eliminate the node >> overhead. I can confirm that it is fill rate >> limited, because when I send the exact same amount of data, but make the >> triangles small, the frame rate goes up as >> expected. It sill looks like it isn't shader limited, though, at least on >> my Windows 10 machine, which has an Intel >> UHD Graphics 630. More testing is needed on other platforms. I'll share the >> mods to the test program when I have time, >> but it's basically just creating a set of quads on top of each other by >> reusing the same 4 points in each pair of faces. > > I'll attach the above modified testcase that I ran. I ran it on a relatively > new Windows 10 laptop and a rather ancient > MacBook Pro. I had to drastically reduce the number of quads on the Mac, but > the results are similar: no significant > difference between the current code and the proposed patch for point lights > (without attenuation). I'd like to see > results on a recent machine with a graphics accelerator (either NVIDIA or > AMD/ATI) to see if the new shader hurts > performance there, but I suspect it will be fine. Updated test case: [attenTest2.zip](https://github.com/openjdk/jfx/files/4332937/attenTest2.zip) - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Tue, 10 Mar 2020 22:41:26 GMT, Kevin Rushforth wrote: >> @kevinrushforth Can you have a look at the test app? I would like to get >> this moving so we would have time to get the >> rest of the lighting enhancements into 15. > > I'll take a look. My quick thought is that we need some sort of test with a > reasonable number of large boxes (so that > it is fill-limited). If there isn't such a case, and the 3D rendering is > always node-limited, then the shader > performance doesn't really matter all that much. I suspect we should be able > to find a case where it does, but we'll > see. I did some limited testing today with a modification to the test program you attached to create a MeshView with 200 large quads (400 triangles) in a single node. This will eliminate the node overhead. I can confirm that it is fill rate limited, because when I send the exact same amount of data, but make the triangles small, the frame rate goes up as expected. It sill looks like it isn't shader limited, though, at least on my Windows 10 machine, which has an Intel UHD Graphics 630. More testing is needed on other platforms. I'll share the mods to the test program when I have time, but it's basically just creating a set of quads on top of each other by reusing the same 4 points in each pair of faces. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Tue, 10 Mar 2020 18:52:23 GMT, Nir Lisker wrote: >> Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't >> complete. An incomplete CSR should be treated >> the same way as an insufficient number of reviewers. I filed >> [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this. > > @kevinrushforth Can you have a look at the test app? I would like to get this > moving so we would have time to get the > rest of the lighting enhancements into 15. I'll take a look. My quick thought is that we need some sort of test with a reasonable number of large boxes (so that it is fill-limited). If there isn't such a case, and the 3D rendering is always node-limited, then the shader performance doesn't really matter all that much. I suspect we should be able to find a case where it does, but we'll see. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Wed, 5 Feb 2020 13:48:45 GMT, Kevin Rushforth wrote: >> We have a few performance tests in apps/performance, but I don't know how up >> to date they are. They might give you a >> starting point on how to measure FPS, but really the harder part is going to >> be coming up with a workload -- a scene >> with a number of Shape3Ds with large triangles (so that they are fill-rate >> limited) and 1-3 lights, etc -- that you can >> use to measure rendering performance without measuring overhead. Typically >> you want a scene that is rendering >> continuously in the < 30 fps range, and more like 10 fps to minimize the >> overhead even more. Before we figure out >> whether we need to double the number of shaders (which was one of the ideas >> that I had as well), we need to know how >> much of a problem it is. Is it < 2% performance drop on typical cases on a >> variety of machines or it is a 25% drop (or >> more likely somewhere in between). If the perf drop is negligible, then it >> isn't worth doubling the shaders. If it is >> significant, then we probably need to. If we do need to double the shaders, >> I wouldn't do it based on the maxRange >> being infinite, rather I would do it based on whether attenuation is being >> used. That way the existing shaders would be >> unchanged, while the new shader would deal with both attenuation and the >> maxRange test. Hopefully, there won't be >> enough of a perf hit to require doubling the shaders, but we need to be able >> to measure it. For functional testing, in >> addition to the simple API tests, we want to make sure that the basic >> rendering is working with and without >> attenuation. Some sort of visual test where you verify that attenuation is / >> isn't happening as well as testing the >> cutoff. I wouldn't get too fancy with these...just a sanity test. > > Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't > complete. An incomplete CSR should be treated > the same way as an insufficient number of reviewers. I filed > [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this. @kevinrushforth Can you have a look at the test app? I would like to get this moving so we would have time to get the rest of the lighting enhancements into 15. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Mon, 17 Feb 2020 01:53:18 GMT, Nir Lisker wrote: >> Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't >> complete. An incomplete CSR should be treated the same way as an >> insufficient number of reviewers. I filed >> [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this. > > I've taken the sample app and enlarged the box to fill the whole range of the > lights in an attempt to have many pixels rendered for few vertices. I > measured 90-115 fps during continuous animation with this patch, and 100-120 > before it. I measured the fps using an external app called BandiCam on Win 10. > > Will do more tests this week. Attaching an attempt at testing performance with and without attenuation. Launch `LightingSample` without the patch or `AttenLightingSample` with the patch. There are 2 modes, one with a single large box and another with many small boxes. Not only is there no difference in performance with and without attenuation, if the lights are turned off (so there is no shader calculation even), there is still no difference. The "single" mode gives ~120 fps, while the "multiple" mode gives ~42 fps and seems to be limited by the number of nodes in the scene rather than anything related to lighting. Either the calculations for 3 lights is negligible, or the testing is flawed. I tested on Win10 with an RX 470 4GB. Used BandiCam to measure the fps. [attenTest.zip](https://github.com/openjdk/jfx/files/4238855/attenTest.zip) - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Wed, 5 Feb 2020 13:48:45 GMT, Kevin Rushforth wrote: >> We have a few performance tests in apps/performance, but I don't know how up >> to date they are. They might give you a starting point on how to measure >> FPS, but really the harder part is going to be coming up with a workload -- >> a scene with a number of Shape3Ds with large triangles (so that they are >> fill-rate limited) and 1-3 lights, etc -- that you can use to measure >> rendering performance without measuring overhead. Typically you want a scene >> that is rendering continuously in the < 30 fps range, and more like 10 fps >> to minimize the overhead even more. >> >> Before we figure out whether we need to double the number of shaders (which >> was one of the ideas that I had as well), we need to know how much of a >> problem it is. Is it < 2% performance drop on typical cases on a variety of >> machines or it is a 25% drop (or more likely somewhere in between). If the >> perf drop is negligible, then it isn't worth doubling the shaders. If it is >> significant, then we probably need to. >> >> If we do need to double the shaders, I wouldn't do it based on the maxRange >> being infinite, rather I would do it based on whether attenuation is being >> used. That way the existing shaders would be unchanged, while the new shader >> would deal with both attenuation and the maxRange test. >> >> Hopefully, there won't be enough of a perf hit to require doubling the >> shaders, but we need to be able to measure it. >> >> For functional testing, in addition to the simple API tests, we want to make >> sure that the basic rendering is working with and without attenuation. Some >> sort of visual test where you verify that attenuation is / isn't happening >> as well as testing the cutoff. I wouldn't get too fancy with these...just a >> sanity test. > > Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't > complete. An incomplete CSR should be treated the same way as an insufficient > number of reviewers. I filed > [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this. I've taken the sample app and enlarged the box to fill the whole range of the lights in an attempt to have many pixels rendered for few vertices. I measured 90-115 fps during continuous animation with this patch, and 100-120 before it. I measured the fps using an external app called BandiCam. Will do more tests this week. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Fri, 10 Jan 2020 00:34:18 GMT, Kevin Rushforth wrote: >>> This will need a fair bit of testing to ensure that there are no >>> regressions either in functionality or (especially) performance, in >>> addition to tests for the new functionality. >> >> Which tests for the new functionality should I write? Up to the shader >> itself it's mostly just passing on variables, and the API is standard >> `DoubleProperty`s. I can test the dirty bits / redraw logic. >> >>> On the performance aspect, the inner loop of the lighting calculation now >>> has an additional if test for the max range and additional arithmetic >>> calculations for the attenuation. What we will need is a test program that >>> we can run on Mac and Windows to measure the performance of rendering in a >>> fill-rate-limited case. Ideally, we would not pay much of a performance hit >>> in the default case where `ca == 1, la == 0, qa == 0`, but we first need to >>> be able to measure the drop in performance before we can say whether it is >>> acceptable. >> >> Can you point me to a similar performance test? I didn't see a way to easily >> measure FPS. >> I don't know how to avoid the `if` test for the `maxRange`, I'm open to >> suggestions. The only thing I can think of is if `maxRange` is infinite (the >> default), we will swap the shader for one that doesn't make that check. >> However, swapping shaders also costs performance. > > We have a few performance tests in apps/performance, but I don't know how up > to date they are. They might give you a starting point on how to measure FPS, > but really the harder part is going to be coming up with a workload -- a > scene with a number of Shape3Ds with large triangles (so that they are > fill-rate limited) and 1-3 lights, etc -- that you can use to measure > rendering performance without measuring overhead. Typically you want a scene > that is rendering continuously in the < 30 fps range, and more like 10 fps to > minimize the overhead even more. > > Before we figure out whether we need to double the number of shaders (which > was one of the ideas that I had as well), we need to know how much of a > problem it is. Is it < 2% performance drop on typical cases on a variety of > machines or it is a 25% drop (or more likely somewhere in between). If the > perf drop is negligible, then it isn't worth doubling the shaders. If it is > significant, then we probably need to. > > If we do need to double the shaders, I wouldn't do it based on the maxRange > being infinite, rather I would do it based on whether attenuation is being > used. That way the existing shaders would be unchanged, while the new shader > would deal with both attenuation and the maxRange test. > > Hopefully, there won't be enough of a perf hit to require doubling the > shaders, but we need to be able to measure it. > > For functional testing, in addition to the simple API tests, we want to make > sure that the basic rendering is working with and without attenuation. Some > sort of visual test where you verify that attenuation is / isn't happening as > well as testing the cutoff. I wouldn't get too fancy with these...just a > sanity test. Looks like the `jcheck` bot removed the `rfr` label because the CSR isn't complete. An incomplete CSR should be treated the same way as an insufficient number of reviewers. I filed [SKARA-262](https://bugs.openjdk.java.net/browse/SKARA-262) to track this. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Thu, 9 Jan 2020 18:00:19 GMT, Nir Lisker wrote: >> The error was for the cases of 2 and 3 lights (I was testing 1) and should >> be fixed now. My fault with copy-paste... that's why we use loops, but I >> guess this is some optimization for the es2 pipeline. I wonder if it's >> really worth it over a single shader looping over the number of lights like >> d3d does. > >> This will need a fair bit of testing to ensure that there are no regressions >> either in functionality or (especially) performance, in addition to tests >> for the new functionality. > > Which tests for the new functionality should I write? Up to the shader itself > it's mostly just passing on variables, and the API is standard > `DoubleProperty`s. I can test the dirty bits / redraw logic. > >> On the performance aspect, the inner loop of the lighting calculation now >> has an additional if test for the max range and additional arithmetic >> calculations for the attenuation. What we will need is a test program that >> we can run on Mac and Windows to measure the performance of rendering in a >> fill-rate-limited case. Ideally, we would not pay much of a performance hit >> in the default case where `ca == 1, la == 0, qa == 0`, but we first need to >> be able to measure the drop in performance before we can say whether it is >> acceptable. > > Can you point me to a similar performance test? I didn't see a way to easily > measure FPS. > I don't know how to avoid the `if` test for the `maxRange`, I'm open to > suggestions. The only thing I can think of is if `maxRange` is infinite (the > default), we will swap the shader for one that doesn't make that check. > However, swapping shaders also costs performance. We have a few performance tests in apps/performance, but I don't know how up to date they are. They might give you a starting point on how to measure FPS, but really the harder part is going to be coming up with a workload -- a scene with a number of Shape3Ds with large triangles (so that they are fill-rate limited) and 1-3 lights, etc -- that you can use to measure rendering performance without measuring overhead. Typically you want a scene that is rendering continuously in the < 30 fps range, and more like 10 fps to minimize the overhead even more. Before we figure out whether we need to double the number of shaders (which was one of the ideas that I had as well), we need to know how much of a problem it is. Is it < 2% performance drop on typical cases on a variety of machines or it is a 25% drop (or more likely somewhere in between). If the perf drop is negligible, then it isn't worth doubling the shaders. If it is significant, then we probably need to. If we do need to double the shaders, I wouldn't do it based on the maxRange being infinite, rather I would do it based on whether attenuation is being used. That way the existing shaders would be unchanged, while the new shader would deal with both attenuation and the maxRange test. Hopefully, there won't be enough of a perf hit to require doubling the shaders, but we need to be able to measure it. For functional testing, in addition to the simple API tests, we want to make sure that the basic rendering is working with and without attenuation. Some sort of visual test where you verify that attenuation is / isn't happening as well as testing the cutoff. I wouldn't get too fancy with these...just a sanity test. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Sat, 4 Jan 2020 18:22:29 GMT, Nir Lisker wrote:
>> Right. This needs to talk about pixels. Perhaps there is a way to make it
>> more clear that we are talking about pixels that are part of a rendered
>> Shape3D, but I don't have a good suggestion right now.
>
> Maybe
>
> A light source that radiates light equally in all directions away from
> itself. The location of the light
> source is a single point in space. The light affects {@code Shape3D}s in its
> {@code scope}. Any pixels in
> the light's {@code range} that belong to a {@code Shape3D} will be
> illuminated by it according to the
> computation specified in {@link PhongMaterial}.
> The docs of `PhongMaterial` will need need to be updated too.
Yes, I think that change to the docs looks good.
-
PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
On Fri, 3 Jan 2020 23:02:56 GMT, Nir Lisker wrote: >>> I still need to test your sample app on Mac. >> >> I get the error with your sample app. It fails on Mac or Linux (Ubuntu >> 16.04) with the same error as I reported above. > > The error was for the cases of 2 and 3 lights (I was testing 1) and should be > fixed now. My fault with copy-paste... that's why we use loops, but I guess > this is some optimization for the es2 pipeline. I wonder if it's really worth > it over a single shader looping over the number of lights like d3d does. > This will need a fair bit of testing to ensure that there are no regressions > either in functionality or (especially) performance, in addition to tests for > the new functionality. Which tests for the new functionality should I write? Up to the shader itself it's mostly just passing on variables, and the API is standard `DoubleProperty`s. I can test the dirty bits / redraw logic. > On the performance aspect, the inner loop of the lighting calculation now has > an additional if test for the max range and additional arithmetic > calculations for the attenuation. What we will need is a test program that we > can run on Mac and Windows to measure the performance of rendering in a > fill-rate-limited case. Ideally, we would not pay much of a performance hit > in the default case where `ca == 1, la == 0, qa == 0`, but we first need to > be able to measure the drop in performance before we can say whether it is > acceptable. Can you point me to a similar performance test? I didn't see a way to easily measure FPS. I don't know how to avoid the `if` test for the `maxRange`, I'm open to suggestions. The only thing I can think of is if `maxRange` is infinite (the default), we will swap the shader for one that doesn't make that check. However, swapping shaders also costs performance. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: [Rev 03] RFR: 8217472: Add attenuation for PointLight
> CSR: https://bugs.openjdk.java.net/browse/JDK-8218264 The pull request has been updated with 1 additional commit. - Added commits: - 66cdab32: Attenuation and range changed internally to floats from doubles Changes: - all: https://git.openjdk.java.net/jfx/pull/43/files - new: https://git.openjdk.java.net/jfx/pull/43/files/464e8b5d..66cdab32 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/43/webrev.03 - incr: https://webrevs.openjdk.java.net/jfx/43/webrev.02-03 Stats: 32 lines in 3 files changed: 0 ins; 0 del; 32 mod Patch: https://git.openjdk.java.net/jfx/pull/43.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/43/head:pull/43 PR: https://git.openjdk.java.net/jfx/pull/43
