Skip to content

Fix raycaster overriding line default color#4743

Merged
dmarcos merged 4 commits intoaframevr:masterfrom
GMartigny:fixLaserPointerColor
Dec 9, 2020
Merged

Fix raycaster overriding line default color#4743
dmarcos merged 4 commits intoaframevr:masterfrom
GMartigny:fixLaserPointerColor

Conversation

@GMartigny
Copy link
Contributor

@GMartigny GMartigny commented Dec 8, 2020

Description:

When creating a raycaster (or a laser-controls for ease of testing), it assigns a default value to the line's color.
It means that the example doesn't works:

<a-entity laser-controls line="color: red"></a-entity>

My fix keep the raycaster's lineColor property for backward compatibility. But in my opinion, it should be deprecated in favor of line's color property.

Changes proposed:

  • Remove default value to raycaster's lineColor property.

Reproduction

<a-scene>
  <a-sky color="#49ACEF"></a-sky>
  <a-entity laser-controls line="color: red; opacity: 0.2"></a-entity>
</a-scene>

The opacity is working but not the color.

@GMartigny
Copy link
Contributor Author

GMartigny commented Dec 8, 2020

For now, I just changed the source-code. If this is accepted, I guess I should update docs and dist ?

@dmarcos
Copy link
Member

dmarcos commented Dec 8, 2020

Using line to set the raycaster color is a brittle since that depends on the internal implementation of raycaster. That could change in the future. Is not the lineColor attribute working for you to customize the raycaster?

@GMartigny
Copy link
Contributor Author

GMartigny commented Dec 9, 2020

Using line to set the raycaster color is a brittle since that depends on the internal implementation of raycaster.

I agree with you. In that case, it should be possible to change the "laser" opacity (without depending on line).

Is not the lineColor attribute working for you to customize the raycaster?

Yes it does. It's juste that the "Customizing the Line" on laser-controls and "Customizing the Line" on raycaster on the documentation are not working.
(I only tested the laser-controls, but both should fail the same way.)

@dmarcos
Copy link
Member

dmarcos commented Dec 9, 2020

What about modifying this PR to add a lineOpacity property to the raycaster component? The docs have to be updated accordingly. Thanks so much for looking into this.

@GMartigny
Copy link
Contributor Author

GMartigny commented Dec 9, 2020

The docs have to be updated accordingly

I don't see any update on the docs yet. I can edit them if you want.

Should I push update to the dist directory also ?

@dmarcos
Copy link
Member

dmarcos commented Dec 9, 2020 •
edited
Loading

Thanks. Can you edit the docs as part of this PR?

The new lineOpacity property has to be also documented in the raycaster component docs below lineColor

Don't push changes on the dist directory

@dmarcos
Copy link
Member

dmarcos commented Dec 9, 2020

Thanks for remembering the tests.

Merci, bien fait!

Congrats on your first contribution! 🥳

@dmarcos dmarcos merged commit 41079d2 into aframevr:master Dec 9, 2020
@GMartigny GMartigny deleted the fixLaserPointerColor branch December 12, 2020 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@GMartigny @dmarcos