decentraland / sdk

PM repository for SDK
Apache License 2.0
4 stars 4 forks source link

Modifiers area hide function doesn't respect excludeId for client #999

Closed drexample closed 10 months ago

drexample commented 1 year ago

Issue Description:

Steps to reproduce:

  1. Create a modifier area with modifiers: [AvatarModifierType.AMT_HIDE_AVATARS]
  2. Add IDs to excludeIds
  3. Observe it hide client's avatar both in first and second person modes

    Expected behaviour:

For the avatars attached to client's ID in excludeIds to not be affected and still be visible

Current behaviour:

It hides user avatar, disregarding it being included in the list of exceptions

Reproduction rate:

100%, currently live at MRT building (Meta Residence Tower) upon entering the lobby. Does not reproduce locally at all.

Severity:

Priority:

Platforms:

Browser:

Environment:

Evidence:

Additional Notes:

Tested AFTER the latest SDK rollout.

aixaCode commented 1 year ago

Could you specify if you are using SDK6 or SDK7?

drexample commented 1 year ago

Could you specify if you are using SDK6 or SDK7?

SDK 7, latest

pravusjif commented 1 year ago

I confirm the problem, I just did a little test and that component is still bugged =(

pravusjif commented 11 months ago

@drexample can you test on your side again? It should be working fine with production's build right now

drexample commented 11 months ago

@drexample can you test on your side again? It should be working fine with production's build right now

Nope, still broken. Even when leaving the area, the avatar keeps being hidden.

yanis7774 commented 11 months ago

@drexample can you test on your side again? It should be working fine with production's build right now

Can confirm. Doesn't work.

pravusjif commented 10 months ago

The feature is working fine when used normally: You can test that yourself by following the test instructions in that feature's latest fix PR description but using production's build instead, with this playground link.

If you correctly enter in a second window using ?ENABLE_WEB3, the client will be able to actually read the wallet address to be able to exclude it (in production, AKA a deployed land, that url param is not needed as that is enabled by default, but not in preview/local mode).

@drexample @yanis7774 if you confirm on your side that those test instructions work fine as they do in my case, you can maybe figure out what you are doing differently in your code, otherwise you can share a code snippet of your implementation here.

yanis7774 commented 10 months ago

The feature is working fine when used normally: You can test that yourself by following the test instructions in that feature's latest fix PR description but using production's build instead, with this playground link.

If you correctly enter in a second window using ?ENABLE_WEB3, the client will be able to actually read the wallet address to be able to exclude it (in production, AKA a deployed land, that url param is not needed as that is enabled by default, but not in preview/local mode).

@drexample @yanis7774 if you confirm on your side that those test instructions work fine as they do in my case, you can maybe figure out what you are doing differently in your code, otherwise you can share a code snippet of your implementation here.

Hi @pravusjif,

The bug is still there. I will attach a video illustrating the bug. In the video, I demonstrate how we exclude avatars by printing the excluded addresses in a console.

Moreover, when I leave the scene, it continues to behave in this manner.

Uploading hidden_avatars_bug_resized.mov…

yanis7774 commented 10 months ago

https://github.com/decentraland/sdk/assets/34626980/f746c19d-4dc7-4944-9bc7-184779209689

pravusjif commented 10 months ago

Uhmm, based on what you show you may be dealing with 2 bugs:

  1. Dynamically changing the excluded ids for the AvatarModifierArea (as you do, using the mutable instance) appears to be bugged
  2. When you leave the a scene whose avatar modifier area occupies entirely, your avatar is not being reset.

I took a quick look on our implementation and both cases should be covered but since they appear to be bugged I'll take a deeper look to debug

yanis7774 commented 10 months ago

Uhmm, based on what you show you may be dealing with 2 bugs:

  1. Dynamically changing the excluded ids for the AvatarModifierArea (as you do, using the mutable instance) appears to be bugged
  2. When you leave the a scene whose avatar modifier area occupies entirely, your avatar is not being reset.

I took a quick look on our implementation and both cases should be covered but since they appear to be bugged I'll take a deeper look to debug

Thank you for the update. We have conducted tests with both dynamic and static configurations for the AvatarModifierArea. In both scenarios, the issue persisted, indicating that the problem might not solely lie with the dynamic changes.

pravusjif commented 10 months ago

I've been able to reproduce the issue consistently: it happens when the user id is added to the avatar modifier area excluded ids collection AFTER the user is already inside the area...

I'll try to find a solution.

pravusjif commented 10 months ago

Fixed at https://github.com/decentraland/unity-renderer/pull/6038

Will reach production with next explorer release + subsequent SDK release

pravusjif commented 10 months ago

Fix released with @dcl/sdk@7.3.36

pravusjif commented 9 months ago

Tried to replicate the problem reported at Meta Residence Tower but I still can't...

Here I tried with 3 different users, 2 of them are in the excluded ids list that I empty/re-fill toggling it with that cube:

https://github.com/decentraland/sdk/assets/1031741/d0e2624b-7207-4516-980e-c06f8d00a19a

const excludedUser1 = 'USER-ID/WALLET-ADDRESS-1'
const excludedUser2 = 'USER-ID/WALLET-ADDRESS-2'
const avatarModifierAreaEntity = engine.addEntity()
const areaSize = Vector3.create(4, 4, 4)
Transform.create(avatarModifierAreaEntity, { 
position: Vector3.create(8, 1, 8),
scale: areaSize
})
AvatarModifierArea.create(avatarModifierAreaEntity, {
area: areaSize,
modifiers: [AvatarModifierType.AMT_HIDE_AVATARS],
excludeIds: []
})
MeshRenderer.setBox(avatarModifierAreaEntity)
Material.setPbrMaterial(avatarModifierAreaEntity, { albedoColor: Color4.create(0.5, 0.5, 0.5, 0.5) })
const cube = createCube(8, 1, 12, false)
MeshCollider.setBox(cube)
pointerEventsSystem.onPointerDown(
  { entity: cube, opts: { button: InputAction.IA_POINTER, hoverText: 'toggle excluded id' } },
  () => {
    const mutable = AvatarModifierArea.getMutable(avatarModifierAreaEntity)
    if (mutable.excludeIds.includes(excludedUser1) && mutable.excludeIds.includes(excludedUser2)) {
      mutable.excludeIds = []
      console.log(`REMOVED excluded users`)
    }
    else {
      mutable.excludeIds = [excludedUser1, excludedUser2]
      console.log(`ADDED excluded users`)
    }
  }
)

function createCube(x: number, y: number, z: number, spawner = true): Entity {
  const entity = engine.addEntity()
  Transform.create(entity, { position: { x, y, z } })
  MeshRenderer.setBox(entity)
  MeshCollider.setBox(entity)
  Material.setPbrMaterial(entity, { albedoColor: Color4.Red() })
  return entity
}

@drexample @yanis7774 can you provide us with an isolated snippet in which you can reproduce the problem?

Otherwise in Meta Tower you could do something like rendering a semi-transparent cube (as I do in the snippet I provided above) to make sure the area is in the correct place with the correct size