[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

Fix raycaster overriding line default color #4743

Merged
merged 4 commits into from
Dec 9, 2020
Merged

Fix raycaster overriding line default color #4743

merged 4 commits into from
Dec 9, 2020

Conversation

GMartigny
Copy link
Contributor

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

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

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

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.

None yet
2 participants
@GMartigny @dmarcos