Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Transfer box customization #171

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

Closed
CrosRoad95 wants to merge 39 commits into multitheftauto:master from CrosRoad95:onTransferBoxStateChange-event
Closed

Transfer box customization #171

CrosRoad95 wants to merge 39 commits into multitheftauto:master from CrosRoad95:onTransferBoxStateChange-event

Conversation

@CrosRoad95
Copy link

@CrosRoad95 CrosRoad95 commented Nov 19, 2017
edited
Loading

This event allow to overwrite currently transfer box into your custom.
Event: onTransferBoxProgressChange arguments: int downloaded, int total ( both in bytes, total == 1024 == 1KB )
functions:

  1. bool setTransferBoxEnabled( bool enabled ) show/hide default transfer box, by default: true
  2. bool isTransferBoxEnabled( ) return true if default transfer box is enabled, false otherwise.

example script that show current progress of downloading on chat, default transferbox doesn't appear

setTransferBoxEnabled(false)
outputChatBox(inspect({"isTransferBoxEnabled",isTransferBoxEnabled()})) -- false
addEventHandler ( "onTransferBoxProgressChange", localPlayer, function( downloaded, total )
 outputChatBox(inspect({"downloading",downloaded,total}))
end )

Then when you restart resource and new resources start downloading, script will show:

restart: Resource restarting...
Press 'o' to open your AC panel
{ "downloading", 0, 30124 }
{ "downloading", 7915, 30124 }
{ "downloading", 8502, 30124 }
{ "downloading", 17474, 30124 }
{ "downloading", 20157, 30124 }
{ "downloading", 28707, 30124 }
{ "downloading", 30124, 30124 }

tested on acpanel above.

Edit.
Added option to give/take server option to control transfer box

image

StrixG, gta191977649, and PlatinMTA reacted with thumbs up emoji Loki214 and PlatinMTA reacted with hooray emoji
example script: https://pastebin.com/Ny153ayg
This event allow to overwrite currently transfer box into your custom. Arguments are current progress and total size in Bytes
Copy link
Member

Also as the event only sends the new current progress and the total size of the transfer and even though onTransferBoxStateChange would be a correct name for it, I feel like onTransferBoxProgressChange would better reflect what it actually sends.
With the current name you gave to that event, (before opening the code changes) I was expecting that you would send something like:

  • "show"
  • "progress", current, total
  • "hide"

But you only do the 2nd point.

Fixed typo,
Removed unneded argument in method `Hide`
Added another argument at the beginning which say status `show` `progress` and `hide`
New example script:
```lua
addEventHandler ( "onTransferBoxProgressChange", localPlayer,function(typ,current,total)
 cancelEvent() -- disable current transfer box
 outputChatBox(inspect({typ,current,total}))
end)
```
output
```
{ "show" }
{ "progress", 446094, 8921880 }
{ "progress", 669141, 8921880 }
{ "progress", 1115235, 8921880 }
{ "progress", 1784376, 8921880 }
{ "progress", 8921880, 8921880 }
{ "progress", 8921880, 8921880 }
{ "hide" }
```

This comment has been minimized.

@Necktrox Necktrox added the enhancement New feature or request label Nov 21, 2017
Copy link
Member

Can we discuss about the name of that event instead of changing it back and forth ?
@Necktrox This event was only sending the current download progress at the begining so onTransferBoxProgressChange was the right name for it.
But now, @CrosRoad95 modified the code of that PR so that it is also sending when the progress bar shows up and when it is hides itself so there are clearly 3 states:

  1. The progress bar shows up, the event is triggered with type="show".
  2. The current progress changes, the event is triggered with type="progress", currentStatus=<currentDownloadProgressInBits>, totalSize=<totalDownloadSizeInBits>
  3. The progress bar hides at the end, the event is triggered with type="hide".

With that in mind, I don't see why we should stick with onTransferBoxProgressChange instead of onTransferBoxStateChange ?

I'm up for any discussion.

Copy link

@Citizen01 I agree, but my code was more about correctness, because the supplied code prior to the changes didn't work.

Furthermore, I would prefer two functions to control/get the visibility of the transfer window instead of cancelling the event.

Copy link
Member

Citizen01 commented Dec 11, 2017
edited
Loading

Furthermore, I would prefer two functions to control/get the visibility of the transfer window instead of cancelling the event.

I agree but if we do so, can the transfer box still flash in the screen if the timing isn't right ?

Copy link
Member

I love the idea of giving scripters some control over the transfer progress GUI, because it is needed to integrate it visually with a scripted, custom GUI system, for example. However, this also allows the potential for scripters to hide the player how much bytes were downloaded from a server, which can be a concern when playing under metered connections. I think it'd be ideal to enforce MTA to display that information no matter what, and then allow scripters to build their own things on top of that if they want to. Maybe some subtle DirectX text in some place can do it when the event is cancelled.

Copy link

@AlexTMjugador Or combine this feature with pull request #154 (Added size column in the server list)

@Necktrox Necktrox added the feedback Further information is requested label Dec 15, 2017
Copy link
Member

AlexTMjugador commented Dec 17, 2017
edited
Loading

That'd be great for me, @Necktrox 👍 . A win-win for everyone.

About the event cancelling, I also think it'd be more appropiate to just provide two functions: one to set the hide status of the transfer window, and another to get it. It's not that hiding it by cancelling the event is incorrect, but in my opinion script logic can get easier the other way. If several scripts want to hide the transfer window, they can call that function once, and if the event triggers then they know that it's because other script enabled it back on. In the case that they didn't have that function, those scripts would have to keep track of the related operations of other scripts more actively to know that, which would lead to more complex and therefore less mantainable code.

Edit: Moreover, if the event is called onTransferBoxStateChange or onTransferBoxProgressChange, I'd expect that cancelling it would somehow undo the circumstance that caused the progress or transfer box state to change, which would essentially mean freezing the download (like cancelling onPickupHit undoes the pickup being picked up). As we definitely don't want that, exposing the ability to hide the GUI by using a separate function seems more clear, too.

Copy link

MIKI785 commented Feb 20, 2018

@Necktrox I understand the concerns for the size of the download still being known to the player, however, the scripter can easily work around this anyway. Simply don't put any files to be downloaded in the meta.xml and then download them using for example triggerLatentClientEvent.
The only way to be sure to see the amount of data being downloaded is to use the shownetstat command.
The reason why I'm posting this is why worry about the scripters hiding the total amount if they already can?

AlexTMjugador reacted with thumbs up emoji

Copy link
Contributor

qaisjp commented Jul 3, 2018

I haven't looked at the code or considered whether this feature is suitable. To answeer one of the things mentioned...

However, this also allows the potential for scripters to hide the player how much bytes were downloaded from a server, which can be a concern when playing under metered connections.

There should be an MTA setting to prevent the server from controlling the transfer box: Allow servers to control the transfer box, default could be true.

With the box unchecked, setTransferBoxEnabled would return false.

CrosRoad95 and AlexTMjugador reacted with thumbs up emoji

@botder botder added refactor review-required and removed feedback Further information is requested labels Jul 9, 2018
CrosRoad95 and others added 6 commits January 2, 2019 14:49
* Create and use new allow_server_control_transferbox setting
* Toggling between custom and native transfer box works by clicking on the checkbox mid-transfer too, unless client is hanging of course
* Add onClientTransferBoxProgressChange event
* Use const where possible
* Use CGUI class as the base for enabled state of transfer box instead of duplicating logic over to CTransferBox (which didn't even work)
* Don't reset transfer total size in Show function, it breaks when toggling the setting off,
* Clean up project comments, clang format
@patrikjuvonen patrikjuvonen dismissed qaisjp’s stale review February 23, 2019 21:49

Changes addressed. New review of course would be great!

@patrikjuvonen patrikjuvonen added the feedback Further information is requested label Feb 23, 2019
Copy link
Contributor

patrikjuvonen commented Feb 23, 2019
edited
Loading

Summary

  • New client setting "Allow server to control transfer box"
    • Enabled by default
  • New event onClientTransferBoxProgressChange with two parameters: int downloadedBytes, int totalBytes
    • This event is only triggered if Allow server to control transfer box setting is turned on
  • New function bool isTransferBoxEnabled ( ) which returns a boolean representing whether the native transfer box is enabled or not
  • New function bool setTransferBoxEnabled ( bool enabled ) which returns a boolean representing whether toggling the native transfer box was successful or not
    • It will return false if the client setting is turned off
  • Native transfer box will toggle visibility accordingly if the transfer box is toggled using setTransferBoxEnabled. This should also happen mid-transfer.
  • Bare in mind that in the current state of the PR, a client hang, which can be caused by the said download, will not necessarily show/hide the native transfer box immediately or at all, until client is responsive again. I don't think this is a problem, since you can create a very small resource to handle resource downloads and that will be able to call the necessary functions before any other transfer. If the setting gets toggled mid-transfer, I don't think that's something we should worry about. This also can be avoided by chunking downloads and using triggerLatentClientEvent for some scripts.

Requesting scripting feedback

  • Let's work towards consensus on what triggers the event, what its name and parameters should be. There have been some great suggestions in this PR, but I would like to bring the discussion back alive and end with a voting of some sorts, considering this PR and discussion is quite old now.

Personally, I don't think this feature is required per se, but I don't mind having it around for more custom loading screens.

Anything else we've missed?

Copy link
Contributor

qaisjp commented Feb 25, 2019

Personally, I don't think this feature is required per se, but I don't mind having it around for more custom loading screens.

My silence on this PR (outside general code review) is due to the fact I am not so sure this feature is a good idea.

@botder botder added this to the Backlog milestone Mar 4, 2019
Copy link

moon91210 commented Mar 11, 2019
edited
Loading

My silence on this PR (outside general code review) is due to the fact I am not so sure this feature is a good idea.

I don't see any reason for concern. People have been requesting this for ages. They just want to create better looking loading screens. Other games support this, take Garry's Mod for example. What's the worst that could happen?

StrixG, Citizen01, ecastro98, DNL291, and Loki214 reacted with thumbs up emoji

Copy link
Contributor

qaisjp commented Apr 26, 2019
edited
Loading

I don't see any reason for concern. People have been requesting this for ages. They just want to create better looking loading screens. Other games support this, take Garry's Mod for example. What's the worst that could happen?

Better loading screens would indeed be nice, and I would be open to discussing such a proposal. However, the functions introduced in this pull request can't be used to implement better looking loading screens as client resources do not initially run until they have all been downloaded.

Thinking about it for a long while now, I don't see any harm in this PR.

Some notes:

... sorry for taking so long...

Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

I merged master in. hopefully i did it correctly. i didn't shift the GUI checkbox around, so someone needs to do that.

Copy link
Contributor

Will we have any update about this?

I think we wouldn't even need a checkbox to confirm whether or not the server can handle the transferbox, i think ideal would be that if the server disables it, it would be visible anyway by pressing esc, that is: allow the servers to use "setTransferBoxEnabled" but this effect would not apply to the main menu

@qaisjp qaisjp removed this from the Confirmed Issues milestone Dec 7, 2020
botder added a commit to botder/mtasa-blue that referenced this pull request Dec 29, 2020
Based on pull request multitheftauto#171 by CrosRoad95
@patrikjuvonen patrikjuvonen removed their assignment Dec 29, 2020
Copy link
Contributor

patrikjuvonen commented Dec 29, 2020
edited
Loading

Let's move to the latest, further and actively developed PR at #1955.

Thank you for your ground work @CrosRoad95

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Citizen01 Citizen01 Citizen01 left review comments

+3 more reviewers

@Necktrox Necktrox Necktrox left review comments

@qaisjp qaisjp qaisjp requested changes

@patrikjuvonen patrikjuvonen patrikjuvonen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request feedback Further information is requested refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /