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

feat: Add Metrics Limits overlay #890

Merged
merged 7 commits into from
Feb 7, 2024

Conversation

cyaiox
Copy link
Contributor

@cyaiox cyaiox commented Feb 6, 2024

@cyaiox cyaiox force-pushed the feat/show-triangles-and-materials-overlay branch from efba29c to fd0c185 Compare February 6, 2024 20:06
Copy link

cloudflare-workers-and-pages bot commented Feb 6, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 22d4d94
Status: ✅  Deploy successful!
Preview URL: https://d171b2ff.js-sdk-toolchain.pages.dev
Branch Preview URL: https://feat-show-triangles-and-mate.js-sdk-toolchain.pages.dev

View logs

@cyaiox cyaiox enabled auto-merge (squash) February 6, 2024 20:07
Copy link
Contributor

github-actions bot commented Feb 6, 2024

Test this pull request

  • The @dcl/sdk package can be tested in scenes by running

    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/show-triangles-and-materials-overlay/dcl-sdk-7.4.1-7821036241.commit-0a78a64.tgz"
  • To test with npx init

    export SDK_COMMANDS="https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/show-triangles-and-materials-overlay/dcl-sdk-commands-7.4.1-7821036241.commit-0a78a64.tgz"
    npx $SDK_COMMANDS init
  • The @dcl/inspector package can be tested by visiting this url

    • Or by installing it via NPM
    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/show-triangles-and-materials-overlay/@dcl/inspector/dcl-inspector-7.4.1-7821036241.commit-0a78a64.tgz"
  • The /changerealm command to test test in-world

    /changerealm https://sdk-team-cdn.decentraland.org/ipfs/feat/show-triangles-and-materials-overlay-e2e
    
  • You can preview this build entering:
    https://playground.decentraland.org/?sdk-branch=feat/show-triangles-and-materials-overlay

@nearnshaw
Copy link
Member

@cyaiox amazing how fast you got this working ❤️

I'm testing in the link to inspector from this PR

it seems that when I change the parcel layout in the component of the Scene entity, this UI doesn't react. It always says 2x2 Land, with limits of a 2x2, even if I add or remove parcels from the list.

Another thing I'm noticing is that you reach the max textures very fast. Most of the items in the asset packs reuse 3 or 4 common textures. But even if I duplicate a same item many times, each copy adds to the number of textures. I should really only count unique textures, repeated textures should only be counted once

Copy link

codecov bot commented Feb 6, 2024

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (d1c5f40) 67.42% compared to head (04d5fd3) 67.30%.

❗ Current head 04d5fd3 differs from pull request most recent head 22d4d94. Consider uploading reports for the commit 22d4d94 to get more accurate results

Files Patch % Lines
...inspector/src/components/Renderer/Metrics/types.ts 0.00% 14 Missing and 1 partial ⚠️
...inspector/src/components/Renderer/Metrics/utils.ts 0.00% 10 Missing and 1 partial ⚠️
...inspector/src/components/Renderer/Metrics/index.ts 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
- Coverage   67.42%   67.30%   -0.12%     
==========================================
  Files         519      522       +3     
  Lines       16064    16093      +29     
  Branches     2048     2051       +3     
==========================================
+ Hits        10831    10832       +1     
- Misses       4904     4929      +25     
- Partials      329      332       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

{showMetrics && (
<div ref={overlayRef} className="Overlay">
<h2 className="Header">
{scene.layout.base.x}x{scene.layout.base.y} LAND
Copy link
Member

Choose a reason for hiding this comment

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

the "COLSxROWS" layout only makes sense for the builder, but for scenes with irregular shapes it does not work. For example you could have a scene with the shape of an "S" and that is not translatable to a COLSxROWS format.

Also layout.base is not being used correctly in this context. The base is the corner of the scene where to originate the x,y,z coordinate system, but is not the number of columns or rows neither.

Also for Worlds the LAND part is not relevant.

So I would say we simply remove the COLSxROWS label from the UI.

@cyaiox cyaiox requested review from cazala February 7, 2024 16:33
Copy link
Member

@cazala cazala left a comment

Choose a reason for hiding this comment

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

I see the metrics do refresh when i add new entities (ie drag n drop or duplicate) but it does not when i delete an entity.

@cyaiox cyaiox merged commit 86c1453 into main Feb 7, 2024
8 checks passed
@cyaiox cyaiox deleted the feat/show-triangles-and-materials-overlay branch February 7, 2024 21:08
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.

3 participants