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

[WIP] [Peripheral] Speaker #201

Closed
wants to merge 42 commits into from
Closed

[WIP] [Peripheral] Speaker #201

wants to merge 42 commits into from

Conversation

Restioson
Copy link
Contributor

@Restioson Restioson commented May 6, 2017

Closed

PR has been closed due to a number of factors. Below is the original PR.

Speaker API

This PR adds a new peripheral: the 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. However, as of writing, speaker.play throws a NPE due to ItemPocketComputer's ServerComputer.getPos() returning null. This is fixed in #172 .

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 tone on a wireless pocket computer every time it gets a packet according to the range of the other modem. You could use this for PVP. I created a working implementation of this. Click here to see the video!

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.

Summation of changes

  • Added speaker_enable config option
  • Added speaker peripheral & provider
  • Added speaker.playsound(resourceLocator, volume, pitch) function in java
  • Added speaker.playnote(noteblockInstrument, volume, pitch) function in java

WIP

Rewrite as peripheral.

Thanks

Special thanks to @SquidDev for helping me understand and navigate the codebase, and @gegy1000 for helping me out with Forge.

@Restioson Restioson changed the title Added SoundAPI [WIP] SoundAPI May 6, 2017
@@ -277,6 +281,9 @@ public void preInit( FMLPreInitializationEvent event )
Config.turtlesCanPush = Config.config.get( Configuration.CATEGORY_GENERAL, "turtlesCanPush", turtlesCanPush );
Config.turtlesCanPush.setComment( "If set to true, Turtles will push entities out of the way instead of stopping if there is space to do so" );

Config.minTimeBetweenSounds = Config.config.get( Configuration.CATEGORY_GENERAL, "minTimeBetweenSounds", minTimeBetweenSounds);
Config.minTimeBetweenSounds.setComment("The minimum time in between calls to sound.play on one computer in milliseconds" );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to add a language key to en_US.lang. Something like gui.computercraft:config.min_time_between_sounds=Minimum time between sounds. Make sure it fits on the screen though. It might also be a good idea to add a .setMinValue(0) so people cannot use negatives.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had added the setMinValue. I don't understand the language key: won't this snippet do it -

for (Property property : Config.config.getCategory( Configuration.CATEGORY_GENERAL ).getOrderedValues())
        {
            property.setLanguageKey( "gui.computercraft:config." + CaseFormat.LOWER_CAMEL.to( CaseFormat.LOWER_UNDERSCORE, property.getName() ) );
        }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will register the language key, but you haven't actually provided a translation - if you open the GUI, you'll just see minTimeBetweenSounds. If you add the appropriate string to en_US.lang, then it should be translated correctly.


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

if (System.currentTimeMillis() - this.lastPlayTime > ComputerCraft.Config.minTimeBetweenSounds.getDouble())
Copy link
Contributor

@SquidDev SquidDev May 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to keep an instance clock variable, incrementing it each time advance is called. This way it'll keep in sync with os.clock() and won't cause issues in the event of server lag. See OSAPI for how it's done there.

Also, store the config value on a field in ComputerCraft when loading the config options - it should be pretty obvious how other config options handle it.


// play
case 0:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap each case statement in braces, so variables are not shared between them. It isn't strictly needed now, but if additional methods are added you'll want them.

/**
* Sound API for ComputerCraft. Provides an interface to Forge's API.
* Possible use cases are notification apis, alert sounds, or playing music
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc should go at the beginning of the class definition, not inside it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, haha! In Python it is the opposite.


private String[] methods;
private ServerComputer computer;
private long lastPlayTime;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dan generally prefixes fields with m_. I don't know how strict he is on this convention, but it might be consistent with the rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to stop using this.member in favour of m_member.

float pitch = 1f;

// Check if arguments are correct
if (arguments.length == 0) // Too few args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just me being petty now, but braces on all the if statements! Unless the condition and body are all on the same line it is, IMO, better to have explicit braces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok. To me it just looked cleaner, but it's just peanuts to me

@Restioson
Copy link
Contributor Author

I've added your suggestions @SquidDev and I fixed the pitch issue. Do you have any other suggestions or shall I remove the WIP tag?

@Lignum
Copy link
Contributor

Lignum commented May 6, 2017

Hmm, I don't think this should be a default API, but a peripheral instead. This way server admins can control specifically who can play sound and who can't, because sounds can be incredibly useful, but also incredibly annoying, even when there's a timeout.

Thanks, @gegy1000 !
@Vexatos
Copy link

Vexatos commented May 6, 2017

I don't think this belongs into CC itself; looks like something a peripheral in an addon should do.

@Restioson
Copy link
Contributor Author

Restioson commented May 6, 2017

@Vexatos why? If this were a peripheral, what would your thoughts be? It is pointedly not a peripheral for use with pocket computers, but now with #172 ... I think it would be super useful to notify people that your computer has done something

@SquidDev
Copy link
Contributor

SquidDev commented May 6, 2017

It is pointedly not a peripheral for use with pocket computers.

Well #172 adds pocket computer peripheral support, so that is less of an issue. If we expand that to allow multiple upgrades, then you will be able to have a noteblock and modem on the same pocket computer.

@Restioson
Copy link
Contributor Author

@SquidDev would it be called a noteblock? It could be a speaker, perhaps.

@Restioson
Copy link
Contributor Author

Restioson commented May 6, 2017

@Lignum do you mean a physical peripheral? Also, your comment about it being annoying - the sound volumedoes drop off over range, very quickly (20~ blocks), limiting the amount that it can be used to grief. How would making it a peripheral make it controllable? Through blacklisting?

@Restioson
Copy link
Contributor Author

I wonder if I could do a permission for sound.

@SquidDev
Copy link
Contributor

SquidDev commented May 6, 2017

Would it be called a noteblock? It could be a speaker, perhaps.

It doesn't really matter, that was an example. I think I've generally seen this method provided via iron noteblocks (such as Peripherals++ or Plethora). Though let's be honest Comptronics's tape drives are way cooler 😄.

@Restioson
Copy link
Contributor Author

Woah, cool! This I feel encourages even more innovation, as you need to invent your own format. Theirs are still cooler though

@Restioson
Copy link
Contributor Author

Restioson commented May 7, 2017

@apemanzilla yeah, it was just a thought. I guess it would be too easy add clay

@Restioson
Copy link
Contributor Author

Hmm, if anyone is good at texturing and wants to try their hand at a speaker peripheral, feel free. I might otherwise just try a retexture of the Wireless Modem with a retextured front.

@dan200
Copy link
Owner

dan200 commented May 7, 2017 via email

@Restioson
Copy link
Contributor Author

Restioson commented May 7, 2017

@dan200 ah, ok. If it were a full block, would turtles not be able to have them? Visually it would look odd. There could be a separate slimmed-down model for the turtles, if they can have them. Will pocket computers be able to have them as per #172 ?

@dan200
Copy link
Owner

dan200 commented May 7, 2017 via email

@Restioson
Copy link
Contributor Author

Restioson commented May 7, 2017

Could that be a retextured modem as I described earlier? It could look like a cut down version of the full block. It'd just use the modem's model.

@dan200
Copy link
Owner

dan200 commented May 7, 2017 via email

@Vexatos
Copy link

Vexatos commented May 7, 2017

You probably shouldn't have tried merging the current master branch into this PR... Normally you would rebase.
grafik
This will likely have some... unwanted side effects.

@SquidDev
Copy link
Contributor

SquidDev commented May 7, 2017

It might be neater to close this PR and re-open a new one with the relevant files copied across - a lot has changed since the original commit and it'll remove the number of merge commits.

I'm going to describe the process I've gone to get my repo set up, just so I can explain the rebasing process. There are probably nicer ways, but this works for me. You've probably done several of these steps already, so skip the ones you've already got:

  • git clone https://github.com/Restioson/ComputerCraft.git: Pretty obvious, clone your repo.
  • git remote add dan200 https://github.com/dan200/ComputerCraft.git Add dan200/ComputerCraft as a remote.

Then assume you create a new branch apiSoundAPI and add some commits. Then this repo's master branch gets some commits, introducing merge conflicts. Then you can do:

  • git remote update Download the latest commits from all all remotes.
  • git rebase dan200/master Rebase this branch onto this repo's master branch. This basically "recreates" all your commits, but using the new master branch as a starting point. You may need to resolve some merge conflicts, but Git guides you relatively well through that.
  • git push -f Push your commits, overwriting the ones already there.
  • ???
  • Profit! And a cleaner git history.

Note that your master branch will also get out of date occasionally. You can just run git pull dan200 master to update it.

@Restioson
Copy link
Contributor Author

Restioson commented May 7, 2017

@Vexatos yeah, that was unintentional.

@dan200
Copy link
Owner

dan200 commented May 7, 2017

Yeah.. this PR is messy now. A peripheral seems a distinct enough thing from an API to warrant a new one anyway.

@Restioson
Copy link
Contributor Author

I'm closing this PR

@Restioson Restioson closed this May 7, 2017
@Wojbie
Copy link
Contributor

Wojbie commented May 7, 2017

On a side note, would you consider integrating a speaker peripheral into the pocket computers by default? It would be very useful for notifications, like on a smartphone.

Ok just throwing a suggestion here but How about splitting the features.
Have all computers (pocket included) [Or just advanced ones only?] have access to basic os.beep(hz) (Like that beeper that is installed on most Motherboards in most computers). Basic beep only, Limited hz range and lasts exactly a second.

Then if someone needs better or more sounds they plug in peripheral Speaker that allows them to
.playNote and .playSound like in that comment by SquidDev.

What you thing about this idea @Restioson. That way we get to have a bird (basic sounds in computers and pockets) and eat a bird (All the sounds in speaker).

@Restioson
Copy link
Contributor Author

@Wojbie sounds good. What do you think @dan200 ?

@Vexatos
Copy link

Vexatos commented May 7, 2017

OpenComputers uses a separate class for synthesizing square waves of a specific frequency manually for its beeps. Should the "motherboard beeps" in CC be square too?

Also, wasn't there something about CC (by default) not exposing anything from "outside the game"? Wouldn't Minecraft sounds count as that?

@Restioson
Copy link
Contributor Author

@Vexatos I think it's more "Minecraft"ey to use noteblock sounds like @SquidDev suggested.

About CC not exposing anything - the sounds are already in the game. The sounds could be recorded, lore-wise (can CC have lore? 😛)

@Vexatos
Copy link

Vexatos commented May 7, 2017

They are in the game, just not by name.

@Restioson
Copy link
Contributor Author

I don't really follow? Do you mean that as a player you cannot play them?

@Vexatos
Copy link

Vexatos commented May 7, 2017

I mean that the names do not appear in-game.

@Restioson
Copy link
Contributor Author

@Vexatos i see what you mean.

@Restioson Restioson mentioned this pull request May 12, 2017
ccserver pushed a commit to ccserver/ComputerCraft that referenced this pull request Sep 16, 2019
This changes the previous behaviour a little, but hopefully is more
sane:

 - Only require the socket to be open when first calling receive. This
   means if it closes while receving, you won't get an error.

   This behaviour is still not perfect - the socket could have closed,
   but the event not reached the user yet, but it's better.

 - Listen to websocket_close events while receiving, and return null
   should it match ours.

See dan200#201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants