Closed SIsilicon closed 3 years ago
The first thing I'm noticing is that this isn't cross-platform and needs to be. This is because for HTML exports, you cannot poll in a loop directly the way you did - those have to be placed essentially in the _process function, because progress cannot be made in the poll() at a rate of faster than a single frame (so everything has to be done on a per-frame basis, basically). However, in theory it should be pretty easy to convert this into that, just gonna take some careful balancing. For more regarding loops and HTML exports: https://docs.godotengine.org/en/stable/getting_started/workflow/export/exporting_for_web.html#class-httpclient-and-class-httprequest - see the third bullet point for more. As far as the rest of the code, I had not, nor had the team, figured out how to do storage at all, so if you can get this working and make it work relatively across exports, we will definitely accept it and thank you very much in the credits. :)
Oki doki! I'll make the necessary changes.👍
On Wed, Jan 27, 2021, 10:20 PM Kyle Szklenski notifications@github.com wrote:
The first thing I'm noticing is that this isn't cross-platform and needs to be. It's not because for HTML exports, you cannot use loops directly the way you did - those have to be placed essentially in the _process function. However, in theory it should be pretty easy to convert this into that, just gonna take some careful balancing. For more regarding loops and HTML exports: https://docs.godotengine.org/en/stable/getting_started/workflow/export/exporting_for_web.html#class-httpclient-and-class-httprequest
- see the third bullet point for more. As far as the rest of the code, I had not, nor had the team, figured out how to do storage at all, so if you can get this working and make it work relatively across exports, we will definitely accept it and thank you very much in the credits. :)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/GodotNuts/GodotFirebase/pull/88#issuecomment-768770060, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIJAAKSVGPSQDH33RE7KRQTS4DJWTANCNFSM4WWEHW2Q .
How's that?
Definitely looks much better! Have you been able to test it and see if it works? I'm really excited for this PR. :)
Uploading, downloading and deleting files with this API works like a dream. There are still some things I need to implement before this PR is ready.
You aren't authenticating to get it to work? I find it to be really weird that it would work without that.
I'm currently testing on my storage with allowed read and write from anywhere. Once I set it to require authentication, I believe it won't work with the current script's state.
The only thing (in theory!) you should need to do when you add authentication is add an authorization header with the appropriate id value from the auth config. You can see how that's done in Firestore here: https://github.com/GodotNuts/GodotFirebase/blob/main/addons/godot-firebase/firestore/firestore_collection.gd#L64
However, it may require some weird form of token exchange - not entirely sure what we're dealing with here.
Ok! It should be feature complete now. I'll do some tests later in the day to make sure that everything's working ok.
Oh, and we should probably think about documentation.
This isn't related to the review I'm currently doing for the code, but this issue: https://github.com/GodotNuts/GodotFirebase-UI/issues/4 should reference this PR, so adding it here (hopefully).
I think this is good to go now! I also made a wiki page that you could use for this. https://github.com/SIsilicon/GodotFirebase/wiki/Storage
Yes, I'm in the process of reviewing. It's going to take a little bit to get merged because we have a big feature on the way as well - are you comfortable with rebase, or would you rather we handle it on our side? I don't think there should be many, if any, conflicts, but it's tough to say ultimately.
I'm fine with doing the rebasing. 👍
It's crazy to me that there's as much code as you wrote and I only found one very small thing to change. That is AWESOME. You are a really good coder, and I really appreciate the PR you've submitted. I'm not sure if what we're working on is going to get done tonight actually, so I may merge this sooner and then we'll handle the rebase ourselves. Looking more at it, I actually don't think there'll be any conflicts at all.
Okay, yeah, I reviewed with the team and I'll merge this as soon as the change to remove the HTTPSEClient is in there. :)
I'm glad I could help out! I think a lot of other Godot users will find this useful for their multiplayer projects. 😁
Congrats! And thank you so much!
Thank you very much for your contribution @SIsilicon
@SIsilicon I'm testing your implementation. Could be able to provide a minimal example to upload an image? (Not necessarely on your wiki, it is fine just here)
Yeah - the major thing we need now is a few good working samples. It's quite possible we just don't know how to use uploading and so it's breaking. @SIsilicon - any chance you could provide one or two of those today (maybe one upload, one download, or perhaps a few different filetypes of each)?
@WolfgangSenff How's this? You can test it and modify it as you wish.
# Must be authorized for this to work.
# If all works well, then this function returns the same texture passed in.
func test_image_storage(texture : Texture) -> Texture:
var storage_ref := Firebase.Storage.ref("testing/image.png")
# Uploading an image
var image := texture.get_data()
var data := image.save_png_to_buffer()
var task := storage_ref.put_data(data)
# Wait until the task is done.
yield(task, "task_finished")
if task.result:
printerr("%s failed to upload!" % storage_ref)
return Texture.new()
else:
print("%s has been uploaded!" % storage_ref)
# Downloading image from the same location
task = storage_ref.get_data()
# Wait until the task is done.
while not task.finished:
print("Download Progress: %s" % task.progress)
yield(get_tree(), "idle_frame")
if task.result:
printerr("%s failed to download!" % storage_ref)
return Texture.new()
else:
print("%s has been download!" % storage_ref)
# Pass the downloaded data into a texture as it was before.
var new_image := Image.new()
new_image.load_png_from_buffer(task.data)
var new_texture := ImageTexture.new()
new_texture.create_from_image(new_image)
return new_texture
Something to note. If you don't pass Content-Type
in the metadata, firebase storage appears to automatically infer the data type according to the file extension.
Edit: You know, perhaps two new functions could be added called put_var
and get_var
that can automatically save and load most Godot data types for you.
@SIsilicon - hmm. I don't think we need those two functions. I love the idea generally, but most types like that aren't things you'd want to store in Storage in my mind. Basically, you want to store the raw data there, but the Godot-types just need to be dynamically generated inside Godot itself. Does that make sense?
Anyway, we'll try out your sample! Thanks!
@SIsilicon Thank you very much for your example.
Actually the problem I had was that I was using image.get_data()
instead of image.save_png_to_buffer()
.
I thought that they were the same function since they both return the PoolByteArray, but apparently they are different. The documentation doesn't provide much about them. Could you explain this difference to me?
@fenix-hub Certainly. You're right that they both return a PoolByteArray
, but the formats of the data are different. get_data()
returns every pixel information in their rawest form, while save_png_to_buffer()
returns the image in a png format; different from the raw pixel data.
@SIsilicon Thank you very much for the explaination :) I've tested your module a lot and it works flawlessly. In the future some documentation will be very useful.
I'm going to release a little PR where I'll add an higher level of communication with your module for signals.
For now you've implemented the task_finished
signal which is a general purpose signal communicating on a lower level - basically it's only emitted by the StorageTask.
This is fine as far as I know that in my project I'll be working only on a specific folder/file, or at least with a small collection.
But if users are going to work with multiple files and folders, and they don't want a non-interrupting communication (signal connection instead of yielding), or connecting for each task a signal, this would be really hard currently.
Of course I could make both task
and storage_ref
variables in a global scope and change their values dinamically, but since with more control on the functions users will end up writing one-line instructions (since it is easier with the logic you have implemented), this is not a good approach.
Hopefully your code is very elastic and modular itself, so the only thing I did was routing the signal emitted from a single task to the Storage module itself.
This will let us have an higher level of communication, following the logic we are currently applying to all other modules, just connecting to the module itself, but without excluding the task itself from the communication:
Firebase.Storage.connect("task_successful", self, "on_task_successful")
with a signal that brings the following parameters:
signal task_successful(result : int, response_code : int, data : Dictionary)
Also I've managed to add a mirroring signal for errors
signal task_failed(result, response_code, data)
I'll release the PR as soon as possible later today, I'll ask for your review so I'll wait for your response. I didn't want to ask you to make this change since you have done already so much work and was just a little thing, but I'll wait for your response to merge this in order to be sure everything is fine with the rest of your module.
This pull request will bring a firebase cloud storage API to this wonderful project. It adds the following features.