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

Add pocket computer light support to Speaker. #289

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add pocket computer light support to Speaker. #289

wants to merge 2 commits into from

Conversation

@Wojbie
Copy link
Contributor

@Wojbie Wojbie commented May 28, 2017

This makes use of new pocket computer light access peripherals have and adds said functionality to speaker. If noisy pocket has made sound the pocket computer light will turn blue for a second.

This showcases use of this feature to future peripheral makes and allows end user to quickly find which pocket in their inventory was making sound.

@Restioson
Copy link
Contributor

@Restioson Restioson commented May 28, 2017

+1, this would be useful.

@Wojbie
Copy link
Contributor Author

@Wojbie Wojbie commented May 29, 2017

Its a little thing but oh so nice. (And would be helpful for people with many pockets. )[<- This close parenthesis is sponsored by @Cruor ]

EDIT: Now a gif cause someone requested it. In this image pocket makes sound every 2 seconds resulting in blink action to showcase color change. Also color changed to lighBlue on request to make it more visible.
2017-05-30_19-38-27

@Restioson
Copy link
Contributor

@Restioson Restioson commented May 31, 2017

I think it could perhaps be dark blue, as this would perhaps contrast more. Play around with the colours!

@Wojbie
Copy link
Contributor Author

@Wojbie Wojbie commented Jun 1, 2017

@Restioson Here is my little test for all colors. Sadly main CC colors don't look nice for this use.
2017-06-01_19-52-02
As for real dark blue here is a test using hex color 0x3320fc (not CC color).
2017-06-01_19-57-24
What do you think?

@Restioson
Copy link
Contributor

@Restioson Restioson commented Jun 2, 2017

@Wojbie I think the green or dark blue is nice. Depends what @dan200 thinks

@KnightMiner
Copy link

@KnightMiner KnightMiner commented Jun 2, 2017

I agree that dark green is the most visible, but dark blue is pretty close and its a color that makes more sense for "is playing sound", as green makes me think "is turned on"

This makes use of new pocket computer light access peripherals have and adds said functionality to speaker. If noisy pocket has made sound the pocket computer light will turn dark blue for a second.
@Wojbie
Copy link
Contributor Author

@Wojbie Wojbie commented Jun 12, 2017

Ok dark blue it is then. Did a squish while at it to not have 3 commits in 5 line PR.

@hugeblank
Copy link

@hugeblank hugeblank commented Jul 28, 2017

Slightly unrelated:
Since this PR, has anyone thought of adding a method that allows the user to directly change the color of the light for the pocket computer? Would be useful for chat programs and the like.

Copy link
Contributor

@Lignum Lignum left a comment

I have some minor nitpicks, but other than that, it looks good to me.

@@ -55,6 +55,11 @@ public BlockPos getPos()
return m_speaker.getPos();
}

public synchronized boolean madeSound()
{
return (m_clock - m_lastPlayTime <= 20) ;

This comment has been minimized.

@Lignum

Lignum Jul 28, 2017
Contributor

This doesn't need to be synchronized because we're exclusively reading from atomic fields. Additionally, I feel it'd be nicer to make that madeSound(long ticks) to get rid of that magical 20 constant.

This comment has been minimized.

@Wojbie

Wojbie Jul 28, 2017
Author Contributor

You really love your functional programing don't you?
But i agree it would make sense to allow for some interesting interaction at later changes.

This comment has been minimized.

@Lignum

Lignum Jul 28, 2017
Contributor

You really love your functional programing don't you?

Yes

@@ -74,6 +75,7 @@ else if ( entity != null )
speaker.setLocation( entity.getEntityWorld(), entity.posX, entity.posY, entity.posZ );
}
speaker.update();
access.setLight( speaker.madeSound() ? 0x3320fc : -1 );

This comment has been minimized.

@Lignum

Lignum Jul 28, 2017
Contributor

This is even more magical :P.

This comment has been minimized.

@Wojbie

Wojbie Jul 28, 2017
Author Contributor

Well this is colors that this one is set to use. Dunno what is your problem with that.

This comment has been minimized.

@Lignum

Lignum Jul 28, 2017
Contributor

0x3320fc is a random number just floating around in here, usually you'd add a comment or make a constant like DEFAULT_LIGHT_COLOUR, but I suppose it's already quite self-evident in this case.

This comment has been minimized.

@Wojbie

Wojbie Jul 28, 2017
Author Contributor

Plus this is exactly same way its coded in modem,

ccserver pushed a commit to ccserver/ComputerCraft that referenced this pull request Sep 16, 2019
…t-computer-light

Add pocket computer light support to Speaker.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants