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

Add uuidv4_text function #42408

Closed
wants to merge 1 commit into from
Closed

Conversation

xsellier
Copy link
Contributor

@xsellier xsellier commented Sep 29, 2020

A the function uuidv4_text() to GDScript, and to the String class

It's been tested on Windows, Mac and Linux

Godot Proposal

@Calinou
Copy link
Member

Calinou commented Sep 29, 2020

Pull requests should be opened against the master branch then cherry-picked to the 3.2 branch, unless they don't apply to tHe master branch in the first place.

@Calinou Calinou added this to the 3.2 milestone Sep 29, 2020
@xsellier
Copy link
Contributor Author

xsellier commented Sep 29, 2020

Pull requests should be opened against the master branch then cherry-picked to the 3.2 branch, unless they don't apply to tHe master branch in the first place.

Sorry about that. I should have opened the PR for the master branch, however I don't have time to do so, so I opened it there, so maybe someone would pick it up, and port it to the master branch, then close this one and cherry-pick to the 3.2 branch.

@Xrayez
Copy link
Contributor

Xrayez commented Sep 29, 2020

Pull requests should be opened against the master branch then cherry-picked to the 3.2 branch, unless they don't apply to tHe master branch in the first place.

Sorry about that. I should have opened the PR for the master branch, however I don't have time to do so, so I opened it there, so maybe someone would pick it up, and port it to the master branch, then close this one and cherry-pick to the 3.2 branch.

For reference, this is what I've been talking about in: #39871 (comment).

Co-authored-by: Hugo Locurcio <hugo.locurcio@hugo.pro>
@vnen
Copy link
Member

vnen commented Oct 12, 2020

Is there any good reason to add this? It could be done with a script.

@xsellier
Copy link
Contributor Author

xsellier commented Oct 12, 2020

I'm using for:

  • Custom database
  • Analytics
  • Bug report

Why it is done via C++ and not GDScript, because the C++ function outperforms GDScript addon (10x faster).

@vnen
Copy link
Member

vnen commented Oct 12, 2020

Sure it's faster, but it doesn't look like performance critical code and you could do it with GDNative to get better performance.

@Zireael07
Copy link
Contributor

If he's using it for a database, then it's absolutely performance critical.

@Calinou
Copy link
Member

Calinou commented Oct 12, 2020

@Zireael07 In a database, you usually have to perform this UUIDv4 conversion only every time you insert an element. Most databases don't even get close to 100 inserts a second 🙂

Also, in many cases, your database can generate an UUID or UUID-like value automatically when inserting a row (see MySQL's uuid()).

@xsellier
Copy link
Contributor Author

xsellier commented Oct 13, 2020

Getting a unique identifier accross the world is useful for a game. Specially if you make a multiplayer game.
Performances matters, Indeed everybody can implement the uuidv4 function on their side, or it could be shared via an addon. It is more convenient to have the uuidv4 function builtin, than having to maintain an addon accross all the godot version (that are not backward compatible)

@Calinou
Copy link
Member

Calinou commented Oct 13, 2020

than having to maintain an addon accross all the godot version (that are not backward compatible)

That's what Git branches are for 🙂

Such an add-on doesn't require a great amount of maintenance. It doesn't interact with any of the editor.

@xsellier
Copy link
Contributor Author

That's what Git branches are for slightly_smiling_face

Such an add-on doesn't require a great amount of maintenance. It doesn't interact with any of the editor.

Why add that work when you can make it built-in and would require no work at all?

@Calinou
Copy link
Member

Calinou commented Oct 13, 2020

Why add that work when you can make it built-in and would require no work at all?

When we add something to core, it shifts the maintenance burden from the user to core developers. Once we add it, we essentially commit to supporting it forever. However, we only have a finite amount of manpower.

I recommend you open a proposal to gather community support on this feature.

@xsellier
Copy link
Contributor Author

xsellier commented Oct 13, 2020

When we add something to core, it shifts the maintenance burden from the user to core developers. However, we only have a finite amount of manpower.

Regarding uuidv4_text, it relies on 3 other functions: Math::rand, OS::get_unix_time and OS::get_ticks_msec.
According to git, those functions haven't changed a lot. So it wouldn't add much work (or even no work at all) to the core developpers.

I recommend you open a proposal to gather community support on this feature.

We know GDScript is slow, so the performance argument raised before is not valid.
We know that uuidv4 is used for a multiplayer game, database, analytics, bug reporting or encryption.
Opening a feature proposal will require more work to the godot core development team, than just merging this PR.

Since there are others PR that have been merged without having any proposals and have a much larger impact than this one, so I don't understand why you are recommanding to open a godot proposal. Also, this PR has more like than most of the PRs I'm quoting.

@Zireael07 In a database, you usually have to perform this UUIDv4 conversion only every time you insert an element. Most databases don't even get close to 100 inserts a second slightly_smiling_face

To generate one ID (average):

  • 0.0106403ms - C++
  • 0.0916400ms - GDScript

Now let's say Im inserting a complex object, so Ill have to generate 10 ids with one action, I wasted ~1ms because of GDScript.

@Xrayez
Copy link
Contributor

Xrayez commented Oct 13, 2020

Once we add it, we essentially commit to supporting it forever.

Nothing is eternal. #42113 is a clear example of this.

However, we only have a finite amount of manpower.

From my observations, we keep gaining more contributors, not less, I'd safely extrapolate that at least 1% of those contributors will become one of those developers who can maintain the engine.

That's what Git branches are for 🙂

There are generally many other smaller features like this which are difficult to keep feature branches for. One has to either use a patching system and/or maintain a custom version of the engine to have persistent, reproducible core functionality across game projects (which also require performance), but that's only applicable for in-house solutions...

Also, I've seen a tendency with Godot development that even the smallest features attract controversial discussions. I don't understand this. I believe this PR is general-purpose enough to cater most needs. By "most" I mean is that, at some point of development, features like this are essential to have in any big project with enough of playerbase, which may not be useful for beginner developers just yet.

Talking about the "adoption" by game development companies across the world as desired by some core developers, alas the current development approach and evaluation of features don't seem to facilitate this.

If Godot Engine caters mostly to beginner use cases, then this should be written on the Godot Engine homepage. But do note that the current wording doesn't reflect this:

Godot provides a huge set of common tools [emphasis mine], so you can just focus on making your game without reinventing the wheel.

@Yukitty
Copy link

Yukitty commented Oct 20, 2020

This should use crypto rand instead of math rand, and the UUID v4 standard does not include timestamps (those are used by other UUID versions)

@PoqXert
Copy link
Contributor

PoqXert commented Dec 5, 2020

Why you add function to String class? Is UUID based on string? What if you want to get this in bytes?

I think that you need to create UUID class with functions for creation UUID v4 and getting it as string. (Other UUID versions may be added to this class.)
For example:

var id := UUID.create_v4();
var id_text := id.to_string();

You can create it as a module and request addition to default modules.

@xsellier
Copy link
Contributor Author

xsellier commented Dec 5, 2020

This should use crypto rand instead of math rand, and the UUID v4 standard does not include timestamps (those are used by other UUID versions)

RFC for UUID v4:
https://www.ietf.org/rfc/rfc4122.txt

 UUID                   = time-low "-" time-mid "-"
                          time-high-and-version "-"
                          clock-seq-and-reserved
                          clock-seq-low "-" node
 time-low               = 4hexOctet
 time-mid               = 2hexOctet
 time-high-and-version  = 2hexOctet
 clock-seq-and-reserved = hexOctet
 clock-seq-low          = hexOctet
 node                   = 6hexOctet
 hexOctet               = hexDigit hexDigit
 hexDigit =
       "0" / "1" / "2" / "3" / "4" / "5" / "6" / "7" / "8" / "9" /
       "a" / "b" / "c" / "d" / "e" / "f" /
       "A" / "B" / "C" / "D" / "E" / "F"

Later in this doc

4.1.2. Layout and Byte Order

To minimize confusion about bit assignments within octets, the UUID
record definition is defined only in terms of fields that are
integral numbers of octets. The fields are presented with the most
significant one first.

Field Data Type Octet Note
#

time_low unsigned 32 0-3 The low field of the
bit integer timestamp

time_mid unsigned 16 4-5 The middle field of the
bit integer timestamp

time_hi_and_version unsigned 16 6-7 The high field of the
bit integer timestamp multiplexed
with the version number

Later in this doc

4.4. Algorithms for Creating a UUID from Truly Random or
Pseudo-Random Numbers

The version 4 UUID is meant for generating UUIDs from truly-random or
pseudo-random numbers.

The algorithm is as follows:

o Set the two most significant bits (bits 6 and 7) of the
clock_seq_hi_and_reserved to zero and one, respectively.

o Set the four most significant bits (bits 12 through 15) of the
time_hi_and_version field to the 4-bit version number from
Section 4.1.3.

o Set all the other bits to randomly (or pseudo-randomly) chosen
values.

@xsellier
Copy link
Contributor Author

xsellier commented Dec 5, 2020

Why you add function to String class? Is UUID based on string? What if you want to get this in bytes?

I think that you need to create UUID class with functions for creation UUID v4 and getting it as string. (Other UUID versions may be added to this class.)
For example:

var id := UUID.create_v4();
var id_text := id.to_string();

You can create it as a module and request addition to default modules.

Since the String class has hash, md5 and sha functions I though it would be a good candidate for uuid:
https://docs.godotengine.org/fr/latest/classes/class_string.html#class-string-method-hash

@xsellier
Copy link
Contributor Author

xsellier commented Dec 5, 2020

Since this PR is dead and it is not going to be merged, Im closing it.

@xsellier xsellier closed this Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants