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

remove all instances of the type long #4519

Merged
merged 1 commit into from
Jan 16, 2022

Conversation

ebbit1q
Copy link
Member

@ebbit1q ebbit1q commented Jan 10, 2022

Related Ticket(s)

Short roundup of the initial problem

the long type has different sizes across operating systems and should
not be used

What will change with this Pull Request?

in the timeline an overflow could occur if the width in pixels
multiplied by the total amount of milliseconds in the replay is larger
than 2^31 which is easy enough considering with only 500 pixels width
you'll reach this number with only 1.2 hours of replay (about 4 million
millis), note that this would be windows exclusive as *nix uses 64 bits

qt-json's own repo suggests using qt5's implementation instead, testing
revealed this is quite a bit faster, contrary to #3480

servatrice uses the qthread usleep function which used to be protected
but is now public

cockatrice is not compatible with qt4 and hasn't been for a while

the long type has different sizes across operating systems and should
not be used

in the timeline an overflow could occur if the width in pixels
multiplied by the total amount of milliseconds in the replay is larger
than 2^31 which is easy enough considering with only 500 pixels width
you'll reach this number with only 1.2 hours of replay (about 4 million
millis), note that this would be windows exclusive as *nix uses 64 bits

~~qt-json's own repo suggests using qt5's implementation instead, testing
revealed this is quite a bit faster, contrary to Cockatrice#3480~~ testing proved
this to not be compatible with older qt versions

servatrice uses the qthread usleep function which used to be protected
but is now public

cockatrice is not compatible with qt4 and hasn't been for a while
@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 10, 2022

after further investigation in became clear that the changes to oracle would not be compatible with older versions of qt, I have split them into a separate branch for now: master...ebbit1q:removeqtjson

@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 10, 2022

I also made a conditional version master...ebbit1q:conditionallyremoveqtjson

@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 10, 2022

@n21lv confirmed that this fixed the issue

@ZeldaZach
Copy link
Member

I want to talk more about this, will DM you

@tooomm
Copy link
Member

tooomm commented Jan 12, 2022

after further investigation in became clear that the changes to oracle would not be compatible with older versions of qt

Shouldn't the QJson Classes be available starting with Qt 5.0?

@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 12, 2022

they can't handle our file sizes, only 5.15 can

@tooomm
Copy link
Member

tooomm commented Jan 12, 2022

I see. Our Debian and Ubuntu are the only ones not using 5.15 yet.
Debian 11 does support it, and Ubuntu 21.04 LTS and newer as well.

@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 12, 2022

ubuntu 21.04 is not an lts release, current lts is 20.04

@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 12, 2022

we should start packaging for debian 11

@tooomm
Copy link
Member

tooomm commented Jan 12, 2022

ubuntu 21.04 is not an lts release, current lts is 20.04

You are right. The releases in April (even years) are the LTS ones, not the uneven. 🙈
Next one will be 22.04 in 3 month.

@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 13, 2022

this is a very simple change, can we merge this so we can make the next beta

@ebbit1q
Copy link
Member Author

ebbit1q commented Jan 14, 2022

I want to talk more about this, will DM you

what do you still need?

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.

Unable to Seek During Replays
3 participants