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

update to aframe master build (post 1.6.0, three r167) to fix undo material src #842

Merged
merged 4 commits into from
Sep 9, 2024

Conversation

vincentfretin
Copy link
Collaborator

update to aframe master build (post 1.6.0, three r167) to fix undo material src (fix #840)

@kfarr
Copy link
Collaborator

kfarr commented Sep 5, 2024

I fear there will be many small issues with an a-frame upgrade, so I'll share them as I find them here:

  • ground textures are too high [1] [2]

[1]
image

[2]
image

@vincentfretin
Copy link
Collaborator Author

ground texture too high, you mean the ocean or the grass on the side? Or is there something else I don't see?

aframe 1.5.0
Capture d’écran du 2024-09-06 10-14-20

aframe master:
image

a tsunami is coming

@vincentfretin
Copy link
Collaborator Author

The position of "Ground right • waterfront" didn't change, so what would be the code to look at for debugging the issue above? wobble-geometry-box maybe?

For the grass I don't see the issue
image

@vincentfretin
Copy link
Collaborator Author

Ah okay it's when it's creating the scene from streetmix json the calculated position for the ground differ.
If I save a go back to aframe 1.5.0, it's still high.
So that would be an issue in the streetmix-loader component.

@vincentfretin
Copy link
Collaborator Author

The water should be at y -3

groundParentEl.setAttribute('position', { y: -3 });

adding there a console.log('groundParentEl', groundParentEl.object3D.position);
I get 0 0 0.
Same with
groundParentEl.setAttribute('position', { x: 0, y: -3, z: 0 });
or
groundParentEl.setAttribute('position', '0 -3 0');
If I do
groundParentEl.object3D.position.setY(-3);
it works. wow. That's new.

@vincentfretin
Copy link
Collaborator Author

Oh the change of position was synchronous in aframe 1.5.0. In aframe 1.6.0 and master (almost 1.7.0 really), the change is done at the end of tick.

groundParentEl.setAttribute('position', { y: -3 }); 
setTimeout(() => {
  console.log('groundParentEl', groundParentEl.object3D.position);
})

shows
{x: 30.55, y: 0, z: 0}

because

groundParentEl.setAttribute('position', { x: groundPositionX });

override completely the value that will be used at the end

@vincentfretin
Copy link
Collaborator Author

In aframe 1.5.0

groundParentEl.setAttribute('position', { y: -3 });
groundParentEl.setAttribute('position', { x: 30.55 });

gives

{x: 30.55, y: -3, z: 0}

In aframe 1.6.0

{x: 30.55, y: 0, z: 0}

The latest setAttribute call wins.

I guess we should review the code were we use an object with only x or y and set the position with groundParentEl.object3D.position directly.

@vincentfretin
Copy link
Collaborator Author

no the setTimeout was already needed in aframe 1.5.0.
I think what changed is that giving a partial position will set the other coordinates to 0 in aframe 1.6.0.

@vincentfretin
Copy link
Collaborator Author

@kfarr you need to explain to me how we get x -30.55 for the left side at the end. Both side are positive:

if (side === 'right') {
// groundParentEl.setAttribute('position', groundPositionX + ' -1 0');
groundParentEl.setAttribute('position', { x: groundPositionX });
} else {
groundParentEl.setAttribute('position', { x: groundPositionX });
}

I really don't see were the x coordinate is flipped afterwards.

@vincentfretin
Copy link
Collaborator Author

So the behavior of using partial object was never intended to work and it's not documented https://aframe.io/docs/1.6.0/components/position.html#main and here it works on aframe 1.5.0 only because it's not attached to the DOM yet. If it's attached to the DOM, setting the position with a partial object will have the other coordinates set to 0.
So with aframe 1.6.0, the behavior is the same, setting the other coordinates to 0, if you add the entity to the DOM before or after setting partial object.

let e = document.createElement('a-entity')
e.setAttribute('position', { y: -3 });
e.setAttribute('position', { x: 30.55 });
AFRAME.scenes[0].appendChild(e)
e.object3D.position
gives {x: 30.55, y: -3, z: 0} only on aframe 1.5.0 but that was an undocumented behavior
gives {x: 30.55, y: 0, z: 0} on aframe 1.6.0

let e = document.createElement('a-entity')
AFRAME.scenes[0].appendChild(e)
e.setAttribute('position', { y: -3 });
e.setAttribute('position', { x: 30.55 });
e.object3D.position
gives {x: 30.55, y: 0, z: 0} on both aframe 1.5.0 and 1.6.0

So we need to modify the code to use el.object3d.position.setY or setX instead or prepare an object and set it with setAttribute('position', obj) in one go. There may be also other part in the code were we set rotation with partial object as well we need to review.

@kfarr
Copy link
Collaborator

kfarr commented Sep 8, 2024

Thanks for finding this. Yes in general the streetmix-parser logic is sloppy and written a long time ago using a variety of techniques. It does need refactoring, thanks for helping with this. I will test now but I assume somehow through magic the object3d is available to set prior to adding the entity to the dom.

@kfarr kfarr merged commit cff7ac3 into main Sep 9, 2024
1 check passed
@vincentfretin vincentfretin deleted the update-aframe branch September 9, 2024 05:25
@vincentfretin
Copy link
Collaborator Author

Thanks for finding this. Yes in general the streetmix-parser logic is sloppy and written a long time ago using a variety of techniques. It does need refactoring, thanks for helping with this. I will test now but I assume somehow through magic the object3d is available to set prior to adding the entity to the dom.

No magic here, the object3d is created right away when you create a-entity
https://github.com/aframevr/aframe/blob/2aca98cbbc5f1f03431ce07f107709baa1718df8/src/core/a-entity.js#L34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

error to undo material texture src change
2 participants