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

Transfer box customization #171

Open
wants to merge 34 commits into
base: master
from

Conversation

@CrosRoad95
Copy link
Contributor

commented Nov 19, 2017

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

onTransferBoxStateChange event
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
@Citizen01

This comment has been minimized.

Copy link
Collaborator

commented Nov 19, 2017

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.

3 patches found by Citizen01
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" }
```
@Citizen01

This comment was marked as resolved.

Copy link
Collaborator

commented Nov 19, 2017

Maybe my english isn't good or you went too fast into making the changes but if you now trigger the 3 states then you should keep onTransferBoxStateChange. I was saying that if you only send the progress, then one would expect onTransferBoxProgressChange as the name for this event.
Sorry for the confusion, can you set the name back to onTransferBoxStateChange ?

CrosRoad95 added some commits Nov 19, 2017

CrosRoad95 added some commits Dec 11, 2017

@Citizen01

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

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.

@Necktrox

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2017

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

@Citizen01

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

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 ?

@AlexTMjugador

This comment has been minimized.

Copy link

commented Dec 14, 2017

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.

@Necktrox

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2017

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

@Necktrox Necktrox added the question label Dec 15, 2017

@AlexTMjugador

This comment has been minimized.

Copy link

commented Dec 17, 2017

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.

@MIKI785

This comment has been minimized.

Copy link

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?

CrosRoad95 added some commits Jul 2, 2018

@CrosRoad95 CrosRoad95 changed the title onTransferBoxStateChange event Transfer box customization. Jul 2, 2018

@qaisjp

This comment has been minimized.

Copy link
Member

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.

@patrikjuvonen
Copy link
Member

left a comment

fix

@CrosRoad95 CrosRoad95 force-pushed the CrosRoad95:onTransferBoxStateChange-event branch from a8b80b2 to 18b1b58 Aug 28, 2018

CrosRoad95 added some commits Aug 28, 2018

fix

@patrikjuvonen patrikjuvonen changed the title Transfer box customization. Transfer box customization Sep 4, 2018

@patrikjuvonen patrikjuvonen self-assigned this Sep 4, 2018

Checks passed.

@patrikjuvonen patrikjuvonen removed their assignment Dec 8, 2018

qaisjp added some commits Jan 1, 2019

Show resolved Hide resolved Client/core/CSettings.h Outdated
Show resolved Hide resolved Client/mods/deathmatch/logic/luadefs/CLuaGUIDefs.cpp Outdated

CrosRoad95 and others added some commits Jan 2, 2019

@patrikjuvonen patrikjuvonen force-pushed the CrosRoad95:onTransferBoxStateChange-event branch from d175f88 to 8eacde0 Feb 23, 2019

Clean up the PR, see further details
* 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

Changes addressed. New review of course would be great!

@patrikjuvonen

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

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?

@patrikjuvonen patrikjuvonen self-assigned this Feb 23, 2019

@qaisjp

This comment has been minimized.

Copy link
Member

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

@moon91210

This comment has been minimized.

Copy link

commented Mar 11, 2019

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?

@qaisjp

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

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:

  • should not be cancellable.
  • a state change with "show", "progress"(+vals), and "hide" (as described here #171 (comment)) seems reasonable.

... sorry for taking so long...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.