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

Cloud Storage API #88

Merged
merged 6 commits into from
Jan 31, 2021
Merged

Conversation

SIsilicon
Copy link
Contributor

@SIsilicon SIsilicon commented Jan 27, 2021

This pull request will bring a firebase cloud storage API to this wonderful project.
It adds the following features.

  • Uploading data to storage
  • Downloading data from storage
  • List directories and files in storage
  • Delete file in storage
  • retrieving and updating file metadata
  • Works with authentication

@WolfgangSenff
Copy link
Collaborator

WolfgangSenff commented Jan 28, 2021

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. :)

@SIsilicon
Copy link
Contributor Author

SIsilicon commented Jan 28, 2021 via email

@SIsilicon
Copy link
Contributor Author

How's that?

@WolfgangSenff
Copy link
Collaborator

Definitely looks much better! Have you been able to test it and see if it works? I'm really excited for this PR. :)

@SIsilicon
Copy link
Contributor Author

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.

  • Make the API work with authentication.
  • Implement more storage reference functions (hopefully).
  • Track upload and download progress.

@WolfgangSenff
Copy link
Collaborator

You aren't authenticating to get it to work? I find it to be really weird that it would work without that.

@SIsilicon
Copy link
Contributor Author

SIsilicon commented Jan 29, 2021

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.

@WolfgangSenff
Copy link
Collaborator

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.

Implemented authentication support,
Added MIME Type constants in StorageRef.
@SIsilicon
Copy link
Contributor Author

Ok! It should be feature complete now. I'll do some tests later in the day to make sure that everything's working ok.

@SIsilicon SIsilicon marked this pull request as ready for review January 30, 2021 18:38
@SIsilicon
Copy link
Contributor Author

SIsilicon commented Jan 30, 2021

Oh, and we should probably think about documentation.

@WolfgangSenff
Copy link
Collaborator

This isn't related to the review I'm currently doing for the code, but this issue: GodotNuts/GodotFirebase-UI#4 should reference this PR, so adding it here (hopefully).

Made some `StorageTask` properties private.
@SIsilicon
Copy link
Contributor Author

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

@WolfgangSenff
Copy link
Collaborator

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.

@SIsilicon
Copy link
Contributor Author

SIsilicon commented Jan 30, 2021

I'm fine with doing the rebasing. 👍

Copy link
Collaborator

@WolfgangSenff WolfgangSenff left a comment

Choose a reason for hiding this comment

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

One very small change.

addons/godot-firebase/storage/storage.gd Outdated Show resolved Hide resolved
@WolfgangSenff
Copy link
Collaborator

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.

@WolfgangSenff
Copy link
Collaborator

Okay, yeah, I reviewed with the team and I'll merge this as soon as the change to remove the HTTPSEClient is in there. :)

The HTTPSSEClient constant was no longer needed.
@SIsilicon
Copy link
Contributor Author

I'm glad I could help out! I think a lot of other Godot users will find this useful for their multiplayer projects. 😁

@WolfgangSenff WolfgangSenff merged commit cdbaf7d into GodotNuts:main Jan 31, 2021
@WolfgangSenff
Copy link
Collaborator

Congrats! And thank you so much!

@fenix-hub
Copy link
Contributor

Thank you very much for your contribution @SIsilicon

@fenix-hub
Copy link
Contributor

@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)

@WolfgangSenff
Copy link
Collaborator

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)?

@SIsilicon
Copy link
Contributor Author

SIsilicon commented Jan 31, 2021

@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.

@WolfgangSenff
Copy link
Collaborator

@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!

@fenix-hub
Copy link
Contributor

fenix-hub commented Feb 1, 2021

@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?

@SIsilicon
Copy link
Contributor Author

@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.

@fenix-hub
Copy link
Contributor

fenix-hub commented Feb 1, 2021

@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.

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.

3 participants