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

What kind of weed was the author of this lab smoking?? #3

Open
miguelcastro67 opened this issue Feb 20, 2020 · 48 comments
Open

What kind of weed was the author of this lab smoking?? #3

miguelcastro67 opened this issue Feb 20, 2020 · 48 comments

Comments

@miguelcastro67
Copy link

@miguelcastro67 miguelcastro67 commented Feb 20, 2020

Jeez, where to start.
The easy thing is fixing the obvious syntax error of -> to =>.
But the walk-through and the code are totally out of sync.
The create a room code is easy enough to put where the comment says, though again, out of sync with the directions. Then it starts to get confusing cause the walk-through talks about adding the code to join a room but doesn't specify where to put it. Adding it immediately after the "create" code ain't working. Another error that comes up is the fact the roomSnapshoot variable is being used but not defined. This whole thing needs to be re-looked at and the author needs some lessons in how to properly teach. Putting the final code somewhere in this repo would be nice.

ErikHellman added a commit that referenced this issue Feb 21, 2020
The comment mentioned in the codelab was missing.

See #3 for details
@ErikHellman
Copy link
Contributor

@ErikHellman ErikHellman commented Feb 21, 2020

The missing comment has been added to app.js at the appropriate place.

Sorry about the mistake of function arrow in the codelab instructions. We'll update that ASAP as well.

@tiloio
Copy link

@tiloio tiloio commented Feb 23, 2020

What about the other confusing comments like:

// Listening for remote session description below
// Listening for remote session description above
// Listen for remote ICE candidates below
// Listen for remote ICE candidates above

// Code for creating SDP answer below
// Code for creating SDP answer above

The Walkthrough (https://webrtc.org/getting-started/firebase-rtc-codelab) has not mentioned that.

@miguelcastro67
Copy link
Author

@miguelcastro67 miguelcastro67 commented Feb 24, 2020

Not to mention the fact that the addition of the code that starts with "const offer = roomSnapshoot..." is very unclear about where to put it. It's in the topic of Join Room, but no clear indication of where it goes. If it's simply added to Create Room, the "offer" var has to be renamed and the roomSnapshot is null, causing an error. This kind of stuff really is rendering this entire walkthrough pretty useless. I'm not a rookie in the business by no means but I am very new to this topic so walkthroughs are incredibly important.

@ErikHellman
Copy link
Contributor

@ErikHellman ErikHellman commented Feb 25, 2020

@miguelcastro67 I hear your concerns, but I don't agree with your opinion that the codelab is useless. There are room for improvements, and we'll continue to work on this.

@miguelcastro67
Copy link
Author

@miguelcastro67 miguelcastro67 commented Feb 25, 2020

@klapperkopp
Copy link

@klapperkopp klapperkopp commented Mar 5, 2020

Completely agree with @miguelcastro67 ! Especially, since this is linked on webrtc.org as "Get Started" guide - assuming no prior knowledge, nearly no beginner would ever get this running.

@miguelcastro67
Copy link
Author

@miguelcastro67 miguelcastro67 commented Mar 5, 2020

Erik, do you have any idea when you'll be posting a working Code Lab? I'm really not going to be able to recommend this technology to my client if I can't educate myself on it; and I can't with the current state of the Code Lab. In all honesty, you need to go through it step by step and I think you'll notice where the lab instructions and the starter code do not sync. Lot's of confusing comments, comments for which you have no implementation, and implementation for which you have no comments. It truly is not workable at all in its current state. Thanks.

@anaseinea
Copy link

@anaseinea anaseinea commented Mar 7, 2020

The best part is where it says "we will secure the database later" and I am still waiting for that, also what about the part of deploying and hosting from firebase ?

@miguelcastro67
Copy link
Author

@miguelcastro67 miguelcastro67 commented Mar 7, 2020

Well I certainly didn’t mean to start a shitstorm but I think it’s easy to see that there are more than just myself that would like to get this usable. It’s a pretty new tech even for seasoned devs. And it’s certainly not intuitive enough that it can just be figured out. I think a good and working walkthrough and sample project is crucial. What do you say, Erik?

@ErikHellman
Copy link
Contributor

@ErikHellman ErikHellman commented Mar 11, 2020

I'm not actively involved in this project myself anymore, so I don't have much time to spend on this. The owners of the project are aware of this and hopefully will address that as soon as possible.

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 17, 2020

The paste in code from the codelab is also filled with the same "->" typo
And the comments are seriously still messed up
This codelab is indeed beneficial but is still a waste of time if the product doesn't work
Please fix this!!!

@py-zoid
Copy link

@py-zoid py-zoid commented Mar 17, 2020

Can anyone please help me out? Where do we even call the collectICECandidates function mentioned in the last step? Also what do we send as the local and remote parameters to the function?

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 17, 2020

Here is the code that works!
https://pastebin.com/rnGGF9rS

Edit: This only fixes the UI not the stream

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 17, 2020

You know, I don't really know where these functions are being called
Also, this works great on my laptop, but when I connect with another device in the LAN then the site can't access the webcam of that device. So I haven't been able to test the actual video chat just the UI....

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 17, 2020

As for the latter part I had to deploy for the scripts to work on other devices
just run "firebase deploy"

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 17, 2020

I think there is an error however with how the collectIceCandidates function is implemented as @py-zoid mentioned. I enter in the same room ID but do not see the other devices' stream...

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 17, 2020

Have you tried looking here? I am trying to just go through their docs and figure it out myself. As it's been over a month since any of the authors/owners made any changes to this repo
https://webrtc.org/getting-started/peer-connections

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 17, 2020

Ok so localName and remoteName are just strings that reference the collection in the room documents, for some reason there are no collections in those documents.
I tried "offer" and "answer" and even offer.spd and answer.spd

I am running the collectIceCandidates function at the end of the joinRoomById function

There are no errors. However, I don't see the stream of other device on my laptop screen and vice versa. There are no errors but no actual connection either. The Friebase database shows that an answer and offer have been sent and recieved. Its a whole mess :(

@cybertheory
Copy link

@cybertheory cybertheory commented Mar 18, 2020

@py-zoid I noticed you deleted a few of your comments... did you fix the issue?

@py-zoid
Copy link

@py-zoid py-zoid commented Mar 23, 2020

@cybertheory I don't think what I mentioned before was the issue. If you just want the solution to this codelab, it's under the solution thread of this repo.

@MickaelRomaniello
Copy link

@MickaelRomaniello MickaelRomaniello commented Mar 27, 2020

Hello, so I was also trying the codelab. I found it absolutely impossible to use.

I was wondering if someone found the good code?
I have checked in the sample but it looks like it's not present.

@PeerRich
Copy link

@PeerRich PeerRich commented Apr 1, 2020

does anyone have a working project incl. server?

@luciana
Copy link

@luciana luciana commented Apr 5, 2020

any updates on this one?

@paper-sahil
Copy link

@paper-sahil paper-sahil commented Apr 6, 2020

yes, the solution branch seems to work, at least locally

@Umair115
Copy link

@Umair115 Umair115 commented Apr 14, 2020

well for me the solution branch also not working, when ever i am creating a room it works fine but when i try to join this room by using its id on another window it shows that roomSnapshot does not exists

@AdrianLopezMuller
Copy link

@AdrianLopezMuller AdrianLopezMuller commented Apr 15, 2020

I'm also facing a similar issue than @Umair115, in the sense that I'm able to create a room and join it once. Attempting to join again after reloading the page or from a different browser/device will perform a PeerConnection creation but not change the status to connected, therefore not displaying any streamed video.

@Umair115
Copy link

@Umair115 Umair115 commented Apr 15, 2020

@powerspowers
Copy link

@powerspowers powerspowers commented Apr 23, 2020

@AdrianLopezMuller I think this sample code is a one use only for each room. The room gets deleted once someone hangs up so you can't rejoin it through a refresh or from another device.

@Umair115
Copy link

@Umair115 Umair115 commented Apr 23, 2020

@AdrianLopezMuller
Copy link

@AdrianLopezMuller AdrianLopezMuller commented Apr 23, 2020

@powerspowers Thanks for the heads up!
Do you know of any other demo where multiple calls can be placed at the same time and reconnecting to the same room is possible? It doesn't need to use firebase in any case.

@Umair115
Copy link

@Umair115 Umair115 commented Apr 23, 2020

@AdrianLopezMuller
Copy link

@AdrianLopezMuller AdrianLopezMuller commented Apr 27, 2020

@Umair115 Hey, that sounds great. Do you mind linking the blogs to take closer look?

@smoreira1
Copy link

@smoreira1 smoreira1 commented May 6, 2020

Though the guide could certainly be improved, this guide did help me learn a lot of core concepts, as well as a lot of ya'll conversation here. Hopefully it can get updated and improved. Not a lot of WebRTC content out. (atleast compared to what I'm typically involved in, Angular, RxJS, Firebase ) so this is pretty much gold. Make it shiner!

@powerspowers
Copy link

@powerspowers powerspowers commented May 7, 2020

I used this code to create https://twisty.io with persistent rooms, room URLs, shorter IDs and either person can start a call. This code project helped a TON.

@miguelcastro67
Copy link
Author

@miguelcastro67 miguelcastro67 commented May 7, 2020

@powerspowers
Copy link

@powerspowers powerspowers commented May 8, 2020

The solution branch in the code works out of the box once you deploy it on Firebase. That helps as you go through the tutorial as a reference.

When you first make a Firebase project the datastore rules are set to allow anyone to read, write objects and collections. Thats obviously not something you want long term so you have to go look up datastore security rules to limit writing to just your domain or if you add authentication (e.g. gmail or facebook which are built into Firebase under the authentication tab) you can limit writing to just logged in users.

Now this codebase doesnt really work for Safari and iOS Safari since they dont seem to like the addTrack calls. I posted the solution to this in another issue so you can pull that fix from there.

Other than those issues the code sample works out of the box and then for me it was just a matter of starting to separate out the room creation from jumping into a room. I also added a shorter ID for naming rooms.

There is a ton more to do to get it where I want it on Twisty (e.g. rejoining rooms, good response if a connection drops, etc) but I found this code solution to be a great start for a simple 1:1 calling system.

If you run into issues keep posting new issues on this project and I will do my best to answer them.

@shahidhafiz
Copy link

@shahidhafiz shahidhafiz commented May 10, 2020

Hi powerspowers, Is twisty made to help us move forward with our own projects. Will you be making available the webrtc implementation please ? atm i believe its in your firebase functions.

@powerspowers
Copy link

@powerspowers powerspowers commented May 10, 2020

All the WebRTC back and forth is in the app.js file actually. There is a lot more that has to be coded to do something intelligent with lost connections and when one party hangs up or tries to reconnect.

The firebase functions create new room IDs and also sort out who is the caller and who is the callee (special signaling logic). My plan is to release a free REST API so that anyone can use it to develop their apps. (preview it by going to https://twisty.io/t/createroom - it will return JSON for success, roomid and roomurl).

You could embed the room URL inside an iframe to use a room within your web site or app.

<iframe allow="camera; microphone; autoplay" src="https://twisty.io/shortid"></iframe>

I think I could do some updates to the original FirebaseRTC project that would help everyone new to coding webrtc. I'll see if I can do a pull request and make some updates.

@Umair115
Copy link

@Umair115 Umair115 commented Jun 3, 2020

@natkuhn
Copy link

@natkuhn natkuhn commented Dec 5, 2020

I see that although there is a Feb 21 comment saying that the "->" would be changed to "=>" in the codelab documentation, here it is almost 10 months later and the same error is still there. The instructions are still a mess--very unclear where things should get pasted. Someone who knows what they are doing could fix the errors, clarify the codelab, and fix the source so that it all fits together in an afternoon, and still have plenty of time to play video games. Not sure what to make of the fact that this seems to have such low priority, but it is too bad.

@natkuhn
Copy link

@natkuhn natkuhn commented Dec 5, 2020

@powerspowers I see a reference to this in another issue, but I don't see a solution posted. Can you let us know where to find it? Thanks

Now this codebase doesnt really work for Safari and iOS Safari since they dont seem to like the addTrack calls. I posted the solution to this in another issue so you can pull that fix from there.

@kemalony
Copy link

@kemalony kemalony commented Dec 5, 2020

I used this code to create https://twisty.io with persistent rooms, room URLs, shorter IDs and either person can start a call. This code project helped a TON.

I could not make it work between laptop and phone

@powerspowers
Copy link

@powerspowers powerspowers commented Dec 6, 2020

@dexterlakin
Copy link

@dexterlakin dexterlakin commented Dec 18, 2020

The solution doesn't work and the arrow function is still broken on master. Waste of time.

@meldaravaniel
Copy link

@meldaravaniel meldaravaniel commented Dec 18, 2020

PSA for @dexterlakin @natkuhn and anyone else coming into this tutorial

I have fixed both the solution and codelab app.js files, as well as rewritten the instructions and added them to the README in my own fork of this project. Waiting on PR's, but I'm going to go bother some folks about it. :)

@natkuhn
Copy link

@natkuhn natkuhn commented Dec 19, 2020

Waiting on PR's, but I'm going to go bother some folks about it.

@meldaravaniel I misunderstood this and thought you were inviting PR's for your repo, so I submitted one. There were still a couple of errors to clean up, and some other suggestions. Thank you so much for doing this, I was finally able to make sense of it!

@meldaravaniel
Copy link

@meldaravaniel meldaravaniel commented Dec 19, 2020

@natkuhn I'm happy to include fixes in my fork. I didn't spend much time fiddling with my own fork, I just wanted it to not frustrate me anymore haha! I'm distracted with adventOfCode instead. :P

@georgegoldman
Copy link

@georgegoldman georgegoldman commented Mar 31, 2021

sincely this what i get after trying to create a room
roomSnapshot is not defined

fadrny pushed a commit to fadrny/FirebaseRTC that referenced this issue Jan 9, 2022
The comment mentioned in the codelab was missing.

See webrtc#3 for details
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

No branches or pull requests