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

[Peripheral] Speaker #237

Merged
merged 20 commits into from May 16, 2017

Conversation

@Restioson
Copy link
Contributor

commented May 12, 2017

Speaker Peripheral

This PR adds the venerable speaker peripheral. This allows computers to play any sound! Theoretically, you can even play sounds from other mods, provided that you use the correct domain.

Possible Use Cases

  • Notification API! You can notify the user with a snazzy ping. Useful as currently there is no way of notifying players of something on a computer
  • Music! You can play records
  • Custom music! You can make music using this by combining different sounds
  • Super sneaky wireless range pinpointer! You can play a ping on a turtle every time it gets a packet, and relay the ping back to home base. You could use this for PVP. (example coming soon)

Example

speaker.playSound("minecraft:entity.cow.ambient") -- Plays entity.cow.ambient from domain minecraft
sleep(1 / 20) -- Sleeps the required amount of time, converting from ticks to seconds
speaker.playSound("entity.cow.ambient", 0.1) -- Plays entity.cow.ambient (in default domain, minecraft) at volume 0.1
sleep(1 / 20)
speaker.playSound("entity.cow.ambient", 1, 0.1) -- Plays entity.cow.ambient at volume 1 and pitch 0.1, thereby slowing it down
sleep(1 / 20)
speaker.playNote("harp", 1, 2) -- Plays the noteblock harp note at volume 1 and pitch 2

Functions

  • speaker.playSound - Takes 1 positional arguments (string resource name) and 2 optional positional arguments (double volume and double pitch). Returns true or false based on whether the resourcename exists and is a sound. Plays the sound at resourcename with volume and pitch, under category RECORDS.
  • speaker.playNote - Takes 1 positional arguments (string instrument) and 2 optional positional arguments (double volume and double pitch). Returns true or false based on whether the instrument exists. Plays the sound at blocks.note.instrument with volume and pitch, under category RECORDS.

Pitching

Pitches begin at 1 (C), and increment by one for each semitone. Therefore, C# is 2, D is 3, etc, up until high C at 24.

Rate limiting

To prevent sound pollution, sounds are limited to once per tick. However, up to 8 notes can be played per tick to allow for chords. If a note is played, then this will not allow a sound to be played in the same tick as well.

Summation of changes

  • Added speaker block
  • Added speaker peripheral
  • Added speaker.playSound(resourceLocator, volume, pitch) function in java
  • Added speaker.playNote(noteblockInstrument, volume, pitch) function in java
  • Added turtle speaker
  • Added pocket speaker

Thanks

Special thanks to @SquidDev for helping me understand and navigate the codebase and with git too many times than I care to admit, and @gegy1000 for helping me out with Forge.

@Wojbie

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

Just a question. If its attached to pocket in your example how are you connecting wireless to turtle with only speaker peripheral attached to pocket? Or is it not attached to pocket in this example?

EDIT: Yea whole:

You can play a tone on a wireless pocket computer every time it gets a packet according to the range of the other modem.

Line is not valid in CC cause of one peripheral limit for pockets (Wish it was two not one but that got shot down already)

@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2017

My video used an outdated version of this PR, back when it was an API -- I'll update it. In any case, the pocket computer wasn't playing the sound: a computer below me was. Thanks for catching it! (If anyone would like to see the vid, see #201)

@Vexatos

This comment has been minimized.

Copy link

commented May 12, 2017

I still think this belongs into its own addon rather than into CC itself. There is little reason for it to be here (in fact, various addons have already done most of what this block does in some way).

@Cruor

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

As you cannot have two peripherals (currently) on a pocket computer, how is the user not able to currently be notified of anything on a computer? You can just hook up a monitor, redstone lamp or noteblock to the computer, which will just as easily notify any players nearby depending on how its set up.

I am not against this being a peripheral, but I don't agree with it being in the main mod, and as @Vexatos says, this functionality is already in many of the existing CC peripheral mods. I believe that a simple beep function, without the need of any peripherals is the best solution, as it would also work with the current peripheral limit on pocket computers.

@Wojbie

This comment has been minimized.

Copy link
Contributor

commented May 12, 2017

Soo first thing first. This is a great peripheral don't get me wrong. But i will agree with the people before me it does feel like it shouldn't be in CC by default.

I also fully agree with @Cruor that getting something like os.beep(Hz) for pockets would be a greatest thing ever since toasted sliced bread.

@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2017

I'll add os.beep, but I don't know whether this should be in the same PR. However, it is still related to sound functionality, so maybe it should be. Maybe it could also just be a harp note to keep it Minecrafty. Thoughts?

@Wojbie

This comment has been minimized.

Copy link
Contributor

commented May 13, 2017

I would suggest separate PR. That way Dan can choose to add both, one, or none.

@@ -323,6 +333,10 @@ public IBlockState getActualState( @Nonnull IBlockState state, IBlockAccess worl
}
break;
}
case Speaker: {

This comment has been minimized.

Copy link
@gegy1000

gegy1000 May 13, 2017

Contributor

Bracket should be on a newline to fit with the rest of the code style

This comment has been minimized.

Copy link
@Restioson

Restioson May 13, 2017

Author Contributor

k

switch (methodIndex)
{
// playsound
case 0: {

This comment has been minimized.

Copy link
@gegy1000

gegy1000 May 13, 2017

Contributor

This { should also be on a newline.

@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2017

I will submit a separate PR in time.

@dan200

This comment has been minimized.

Copy link
Owner

commented May 14, 2017

Great work! This has been implemented really well, the API is nice, and even the block graphics look nice! Bravo. I only have one thing I need changed, which I will add in a comment in a minute

Just a note for anyone adding new peripherals in the future though: You don't have to make your peripheral a subtype of a "peripheral" block if you don't want to: it's ok to register a new block ID. Combining them all into one block is a legacy (and a backwards compatibility constraint) of the days when we only had 255 block IDs to play with for the whole game!

}

@Override
public Object[] callMethod(IComputerAccess computerAccess, ILuaContext context, int methodIndex, Object[] args) throws LuaException

This comment has been minimized.

Copy link
@dan200

dan200 May 14, 2017

Owner

So here's my issue with this PR: All the code called from this method is completely non-thread safe. Methods called from Lua always happen on the Lua thread, so it's not safe to call methods on anything managed by the main world directly unless it's your own data that you're correctly managing access to with syncronized blocks.

My recommendation is to use ILuaContext.issueMainThreadTask to ensure your sounds are played on the main thread. It's still safe and recommended to check all your parameters are correct before making this call so you can earlyout and give appropriate errors if they're incorrect.

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

Thanks! I'll do this today, and hopefully resolve the conflicts with upstream.

@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

Credit @gegy1000 for model though. (I can't model)

@Restioson Restioson force-pushed the Restioson:feature/speaker branch from a1b7f88 to c643910 May 15, 2017
@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

As far as I am concerned, this PR is ready for merge. EDIT: it's not, nevermind.

registerTileEntity( TileCable.class, "wiredmodem" );
registerTileEntity( TileCommandComputer.class, "command_computer" );
registerTileEntity( TileAdvancedModem.class, "advanced_modem" );
GameRegistry.registerTileEntity( TileComputer.class, "computer" );

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

Please change these calls to use the "registerTileEntity" call on the left

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

I must have accidentally overwritten this in the merge.

@@ -325,6 +335,8 @@ public static void syncConfig() {
turtlesObeyBlockProtection = Config.turtlesObeyBlockProtection.getBoolean();
turtlesCanPush = Config.turtlesCanPush.getBoolean();

maximumFilesOpen = Math.max(1, Config.maximumFilesOpen.getInt());

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

I presume this was meant to be maxNotesPerTick?

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

Yeah. I meant to change that. Sorry!

@@ -0,0 +1,45 @@
/*

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

It wasn't necessary to create a whole new class for this. Use an anonymous class.

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

The issue with this is that I can't use a constructor in an anonymous class, which is currently how I pass the args initially.

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

I've never found a need to put a constructor on an anonymous class. Just "construct" things in the enclosing scope.

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

Would I not need to make them final then?

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

Alternatively, I could use an inner class.

This comment has been minimized.

Copy link
@SquidDev

SquidDev May 15, 2017

Contributor

You'd have to make them final. For volume and pitch you can always create final copies of the original variable. I don't think the other three are mutated.

This comment has been minimized.

Copy link
@Restioson

Restioson May 16, 2017

Author Contributor

That is true. I'll probably do that.

throw new LuaException("Invalid instrument, \"" + arguments[0] + "\"!");
}

if (arguments.length > 1)

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

This will fail if you pass (string, nil, nil), when it probably shouldn't (this commonly happens when you wrap a method in Lua). You should do null checks

This comment has been minimized.

Copy link
@SquidDev

SquidDev May 15, 2017

Contributor

Slightly off topic, but what would your thoughts be on abstracting all argument validation out into a helper class? One thing which might be nice would be including the provided type, like standard Lua functions do (so bad argument #1 (string expected, got nil) instead of Expected string, string).

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

To me, or to Dan? I would be happy to do this in my PR

This comment has been minimized.

Copy link
@SquidDev

SquidDev May 15, 2017

Contributor

This was directed at Dan. Sorry for hijacking another thread, I just wanted to put out some feelers before creating a full blown issue/PR.


}

if (arguments.length > 2)

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

Same as above

pitch = ((Double) arguments[2]).floatValue();
}

if (arguments.length > 3)

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

I would just remove this check. it's perfectly okay in Lua style to pass extra stuff on the end that isn't used

throw new LuaException("Expected string, number (optional), number (optional)");
}

if (arguments.length > 1)

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

See comments in playNote


ResourceLocation resourceName = new ResourceLocation((String) arguments[0]);

if (m_clock - m_lastPlayTime >= TileSpeaker.MIN_TICKS_BETWEEN_SOUNDS || ((m_clock - m_lastPlayTime == 0) && (m_notesThisTick < ComputerCraft.maxNotesPerTick) && isNote))

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

You're still accessing a lot of variables in a thread unsafe way here.. it might be fine, but it's not guaranteed

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

Do you have any suggestion as to how I could improve this? I do not have much experience with threads in Java, but do have some in other languages. I generally steer clear of them

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

Should I used synchronized?

This comment has been minimized.

Copy link
@SquidDev

SquidDev May 15, 2017

Contributor

Yes. You can probably mark the entire method as synchronized. I don't know if TilePeirpheral.update also needs to lock though.

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

I'll do it just to be safe.


package dan200.computercraft.shared.peripheral.speaker;

import dan200.computercraft.api.peripheral.IPeripheral;

This comment has been minimized.

Copy link
@dan200

dan200 May 15, 2017

Owner

Bad whitespace

This comment has been minimized.

Copy link
@Restioson

Restioson May 15, 2017

Author Contributor

Odd, must've accidentally hit tab a few times

@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

I'll try push fixes to @dan200 suggestions tomorrow, when I have time.

@dan200

This comment has been minimized.

Copy link
Owner

commented May 15, 2017

If you like, feel free to fix everything except the multithreading concerns, and i'll push a fix to them myself, might be easier than a long back and forth.

Push your fix to thread safety if this doesn't cut it
@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

@dan200 I've hopefully fixed everything including the multithreading using synchronized methods. If this doesn't cut it, merge and push your own fixes 😄


if (arguments.length > 1)
{
if (!(arguments[1] instanceof Double) && !(arguments[1] == null)) // Arg wrong type

This comment has been minimized.

Copy link
@SquidDev

SquidDev May 16, 2017

Contributor

To be really fussy, it should probably be arguments[1] != null. Similarly below. However, you could probably shift this up above so it's if(argument.length > 1 && arguments[1] != null) { /* Do double check/volume assignment here */ }.

This comment has been minimized.

Copy link
@Restioson

Restioson May 16, 2017

Author Contributor

@SquidDev I agree with the first part, but to make it optional, I'd need to have another block for arguments.length > 1 && arguments == null { /* Do default here */ } whereas previously I could just use a ternary.

This comment has been minimized.

Copy link
@SquidDev

SquidDev May 16, 2017

Contributor

Isn't this OK? I might be misunderstanding you. It's pretty much what you have now but without the ternary.

if(arguments.length > 1 && arguments[1] != null) {
    if(!(arguments[1] instanceof Number) {
        throw new LuaException("Expected string, number (optional), number (optional)");
    }
    volume = ((Number)arguments[1]).floatValue();
}

You've already got the default value there, so you shouldn't need it again.


if (returnValue[0] instanceof Boolean && (Boolean) returnValue[0])
{
m_notesThisTick++;

This comment has been minimized.

Copy link
@dan200

dan200 May 16, 2017

Owner

This seems to only be incremented in playNote, not playSound. Surely this means playSound() isn't rate limited?

This comment has been minimized.

Copy link
@Restioson

Restioson May 16, 2017

Author Contributor

This is intentional. It is rate limited, once per tick. However, to allow for chords, playNote has 8 times per tick. Could be switched to both being 8 times per tick, but could have sound spam, whereas notes are short.

This comment has been minimized.

Copy link
@dan200

dan200 May 16, 2017

Owner

Ahh, I didn't notice the "isNote" check. Fair enough. Merging!

@dan200

This comment has been minimized.

Copy link
Owner

commented May 16, 2017

Getting there! Just left a comment on one possible bug.

@dan200

This comment has been minimized.

Copy link
Owner

commented May 16, 2017

This is ready to merge :) Before I do though, could you please add the change to the help system. "whatsnew" and "changelog" are the files to modify.

Restioson added 2 commits May 16, 2017
@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

On an unrelated note, in changelog, it says "Pocket upgrde api", and not "upgrade".

@SquidDev

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

Oooooh, sorry about that. Apparently it doesn't matter how many times I check spelling, something will always get through. Sorry.

On an equally unrelated note there are a couple of other changelog items which probably need clearing out.

@dan200

This comment has been minimized.

Copy link
Owner

commented May 16, 2017

feel free to fix it

@dan200 dan200 merged commit cd85a03 into dan200:master May 16, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

Yay!

@dmarcuse

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

Now that speakers are in, the dj program could probably use a revamp.

@Restioson Restioson deleted the Restioson:feature/speaker branch May 20, 2017
@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

@apemanzilla 😛

@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

Hey, it'd be cool if there was a sound file format, like MIDI but for CC. ccmid? Just mid? Not at a CC level, but at a community level. Being said, if one was written, it'd be fun if dj could play one...

@SquidDev

This comment has been minimized.

Copy link
Contributor

commented May 20, 2017

There are several players for the Note Block Studio format, such as BombBloke's Note program*. This is pretty much the defacto standard,

*Yes, there are lots of others. This was the first one which sprung to mind so don't yell at me for not mentioning yours.

@Restioson

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2017

Hate to necro here, but we appear to have an issue: the lack of documentation on the wiki, or anywhere for that matter. I'll edit the PR with more info about the pitching and volume, etc, and then someone (or me, but I'd need to gain wiki editing privileges) can edit them.

@SquidDev

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

I've posted this on Discord, but just adding this here for consistency too: RE documentation, it might be worth having a read through #271 and the documentation related discussion in #343 (comment). Basically, no one is entirely sure whether we should put documentation on the wiki until we have a formal release or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.