Skip to content

Conversation

@arvind-iyer
Copy link
Contributor

Description:
Instead of the current stats implementation, here I have used stats-gl for the performance statistics as discussed in the issue thread #5397

Changes proposed:

  • Modify components/scene/stats.js to use stats-gl
  • Add stats-gl to the package.json
  • Remove previously used rStatsAframe.js file

Updates to documentation have not been done here yet

@mrxz
Copy link
Contributor

mrxz commented Apr 6, 2025

Thanks, nice to have a simpler stats component. There are still a couple more files related to rStats that can be removed as well, namely: vendor/rStats.js, vendor/rStats.extras.js and src/style/rStats.css. As well as the line in index.js including the .css file:

require('./style/rStats.css');

Functionally there is an annoying edge-case. With post-processing we override the render method similar to what stats-gl does under the hood. But this means that the order in which the components initialize matter. I'm wondering if we should perhaps manually call stats.begin()/.end()/.update() from scene.js. This would avoid that issue as well as include the .tick/.tock execution time in the CPU metric.

@dmarcos
Copy link
Member

dmarcos commented Apr 8, 2025

Can we remove the unnecessary files? Can we deal with post-processing separately ideally without coupling with a-scene?

@arvind-iyer
Copy link
Contributor Author

I have cleaned up the unused rStats files for now, but the alternative approach mentioned by @mrxz involves a larger overhaul. I don't think I understand how that would be done or what implications it would have.

@mrxz
Copy link
Contributor

mrxz commented Apr 9, 2025

No worries, it's more thinking out loud than anything concrete. The way post-processing is done leads to conflicts with components that patch/hook into rendering (screenshot (#5677) and stats). Ideally there'd be a clean interface for these components provided by a-scene as it controls the render loop.

The fact that the measured CPU time does not include any system or component logic is a bigger issue IMO. Users would expect it to reflect those as well. But merging this PR as-is, is fine. The above mentioned issue is something that needs to be addressed anyway, at which point the stats can be updated as well.

@dmarcos
Copy link
Member

dmarcos commented Apr 9, 2025

Needs the tests fixed

@dmarcos
Copy link
Member

dmarcos commented Apr 15, 2025

Thanks so much!

@dmarcos dmarcos merged commit 1595b07 into aframevr:master Apr 15, 2025
3 checks passed
@vincentfretin
Copy link
Contributor

vincentfretin commented May 24, 2025

@arvind-iyer removing the stats component doesn't properly remove the two graphs, it just freeze them. You can test in the console with AFRAME.scenes[0].removeAttribute("stats"); Can you please look at it?

@vincentfretin
Copy link
Contributor

The number of draw calls was an interesting metric for me, but I can still access the stats with a console.table:

console.table(AFRAME.scenes[0].renderer.info.memory);
console.table(AFRAME.scenes[0].renderer.info.render);

@diarmidmackenzie your stats-panel component will probably be broken by this change, you may want to get back some of the previous code into your component.

@vincentfretin
Copy link
Contributor

@arvind-iyer removing the stats component doesn't properly remove the two graphs, it just freeze them. You can test in the console with AFRAME.scenes[0].removeAttribute("stats"); Can you please look at it?

I fixed it in #5733

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