/WurstStdlib2

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

Network, Execute fixes & FileIO #26

Merged
merged 25 commits into from Jan 29, 2018

Conversation

Reviewers

@Cokemonkey11 Cokemonkey11

@Frotty Frotty

Assignees

@Cokemonkey11 Cokemonkey11

@Frotty Frotty

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@SamuelMoriarty @Cokemonkey11 @Frotty
@SamuelMoriarty
Copy link
Contributor

SamuelMoriarty commented Jan 3, 2018

No description provided.

@Frotty

Frotty reviewed Jan 6, 2018
View changes

Copy link
Member

Frotty left a comment •
edited

I still need to test it in ebr, but looks sweet so far 👍
wurst/closures/Execute.wurst
function getCurrentCallback() returns ForForceCallback
return tempCallbacks[tempCallbacksCount - 1]

/**
This function starts a new 'thread' inside the

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

I think instead of resetting the oplimit the newly created thread simply starts at zero.
Other important points to illustrate would be the difference between a timer/doAfter (the thread that starts the execute waits for it to be finished) and why we use ForForce instead of TriggerEvaluate.
wurst/closures/Execute.wurst Outdated
tempCallbacksCount++

function popCallback()
tempCallbacksCount--
destroy tempCallbacks[tempCallbacksCount]

function getCurrentCallbackSuccess() returns boolean

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

Should be "isCurrentCallbackSuccess(ful)"
wurst/closures/Execute.wurst Outdated
public function execute(ForForceCallback c)
pushCallback(c)
executeForce.forEach(function executeCurrentCallback)
let success = getCurrentCallbackSuccess()

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

with "isCurrent.." I think this variable could be inlined
wurst/closures/Execute.wurst Outdated
interface LimitedExecuteAction
function run()

public function limitedExecuteInternal(int iterations, LimitedExecuteCondition condition, LimitedExecuteAction action)

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

I'm not really happy with the name. Limited to me implies that it is more "limited" compared to the normal execute. But in actuality it does more than the normal execute.
I think something like Periodic, Sequential or Batch - Execute would be more intuitive.
wurst/closures/Execute.wurst Outdated
popCallback()

interface LimitedExecuteCondition
function check() returns boolean

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

The problem i see with the condition is that it doesn't have access to the iteration variable i which you might want to base your condition on instead of providing a fixed amount.
Maybe provide 1 execute that takes iterations and action, and one that only takes condition and action, where the condition will also check the iteration count by receiving it as parameter.

This comment has been minimized.

Sign in to view
Copy link
@SamuelMoriarty

SamuelMoriarty Jan 13, 2018

Author Contributor

The 'iterations' here is a completely internal variable that is used to reset the OP limit when iteration count is exhausted. This value can't be used to control the outer loop, because it gets reset to 0 each iterations times.

Without a condition, this function will simply loop forever. The condition is used to break the loop.
wurst/file/IOTaskExecutor.wurst Outdated
for executor in TimedIOTaskExecutor
executor.update()

// called when an executor instance has started working

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

convert to hot doc, also the ones below
wurst/file/IOTaskExecutor.wurst Outdated
lastExecution = currentTime
if finished
runningCount--
// stop the timer if necessary

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

"if necessary" ?
wurst/file/Persistable.wurst

construct(player owner)
this.owner = owner
store = new PersistentStore(this, owner)

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

is the store ever destroyed?
wurst/file/Persistable.wurst Outdated
protected function getPath() returns string
return PERSISTABLE_MAP_PREFIX + "/" + getClassId() + "/" + getId()

/* You must implement these */

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

then they should probably be at the top :P
wurst/file/Persistable.wurst Outdated
buffer.populateFromFile(reader)
buffer.transfer(network.getData())
// if there was an error during transfer, we scrap the data...
if not buffer.isGood()

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 6, 2018

Member

"isGood" should be replaced by something more descriptive

@SamuelMoriarty SamuelMoriarty force-pushed the SamuelMoriarty:persistence branch from b151787 to 85a2798 Jan 13, 2018

@Cokemonkey11

Cokemonkey11 reviewed Jan 15, 2018
View changes

wurst/_handles/Playercolor.wurst
if lookup[i] == this
value = i
break
return value

This comment has been minimized.

Sign in to view
Copy link
@Cokemonkey11

Cokemonkey11 Jan 15, 2018

Contributor

missing eof lf
@Cokemonkey11

Cokemonkey11 reviewed Jan 15, 2018
View changes

wurst/closures/Execute.wurst Outdated
function executeWhileInternal(int resetCount, LimitedExecuteCondition condition, LimitedExecuteAction action)
execute(() -> begin
var i = 0
// iterate as many times as needed

This comment has been minimized.

Sign in to view
Copy link
@Cokemonkey11

Cokemonkey11 Jan 15, 2018

Contributor

comment not useful
@Cokemonkey11

Cokemonkey11 reviewed Jan 15, 2018
View changes

wurst/data/Buffer.wurst
@@ -565,16 +579,33 @@ public class StringBuffer extends AbstractBuffer
checkRead()
return checkType(ValueType.BOOLEAN) ? (popString(1) == "1") : false

override function clear()
for chunk in chunks
destroy chunk

This comment has been minimized.

Sign in to view
Copy link
@Cokemonkey11

Cokemonkey11 Jan 15, 2018

Contributor

\t ⁉️
@Cokemonkey11

Cokemonkey11 reviewed Jan 15, 2018
View changes

wurst/file/MultiIO.wurst
If multiMode is true, then path should be the path to a folder where the data will be stored.
Otherwise, the path should be a file path.

Delay is the number of seconds to wait between each file write operation. You should use

This comment has been minimized.

Sign in to view
Copy link
@Cokemonkey11

Cokemonkey11 Jan 15, 2018

Contributor

\t ⁉️
@Cokemonkey11

Cokemonkey11 reviewed Jan 15, 2018
View changes

wurst/file/Persistable.wurst
You should override this to prevent conflicts with other maps using this class.
**/
@configurable
constant PERSISTABLE_MAP_PREFIX = "persistent"

This comment has been minimized.

Sign in to view
Copy link
@Cokemonkey11

Cokemonkey11 Jan 15, 2018

Contributor

needs comment
@Cokemonkey11

Cokemonkey11 reviewed Jan 15, 2018
View changes

wurst/file/SimpleIO.wurst
/** Maximum amount of packets per single readable file. */
public constant PACKETS_PER_FILE = 16
/** Maximum payload size of a single readable packet. */
public constant MAX_PACKET_LENGTH = 209

This comment has been minimized.

Sign in to view
Copy link
@Cokemonkey11

Cokemonkey11 Jan 15, 2018

Contributor

magic number?
@Cokemonkey11

This comment has been minimized.

Sign in to view
Copy link
Contributor

Cokemonkey11 commented Jan 15, 2018

Overall this is pretty brilliant. I think it's the biggest PR on stdlib. I hope Frotty has a backup of this code in case something happens.

As it stands, the code quaility is very high and I think you've balanced perfection and pragmatism well.

The nature of this stuff is fragile, though. There's necessarily a lot of non-generic type stuff here, bit twiddling, magic values, data reinterpretation, etc. I think that's fine.

My final opinion is that this could go two ways:

  • stdlib isn't "stable", so this fragile code can get merged just fine without anyone worrying. After all, NASA is not writing wurst code. In this route, I think this MR needs some squashing/otherwise interactive rebasing.

  • This huge, fairly specialised, fragile code probably could be in a separate lib - which is a great opportunity for us to play with dependency manager, and otherwise just makes sense.

I'll let you and Frotty decide which of these variants is the right one. In any case, please someone get a backup, and also thanks again, this is really, really cool 😎
@Frotty

This comment has been minimized.

Sign in to view
Copy link
Member

Frotty commented Jan 15, 2018 •
edited

I agree. I think it could be in stdlib to promote easy persistable data.
On the other hand, most players don't even have local files enabled, making this impractical for use in "any map".
If loading will ever be possible without local files, this would be killer stdlib feature tho imo

e: Just as a reminder to @SamuelMoriarty the missing features we talked about;

  • Detection if Local Files are enabled by attempting to load a probe file at the beginning of the game
  • 1.26 fallback solution that uses the Logs// folder inside the gamedir for Preloader data

@Frotty Frotty assigned Frotty and Cokemonkey11 Jan 17, 2018

@SamuelMoriarty SamuelMoriarty force-pushed the SamuelMoriarty:persistence branch from 85a2798 to 4c19903 Jan 29, 2018

@SamuelMoriarty
Quick fix
Loading status checks…
eae5e73

@SamuelMoriarty SamuelMoriarty force-pushed the SamuelMoriarty:persistence branch from e2c1718 to eae5e73 Jan 29, 2018

@Frotty

Frotty reviewed Jan 29, 2018
View changes

wurst/_wurst/ErrorHandling.wurst Outdated
@@ -8,10 +8,16 @@ import GameTimer
import MagicFunctions
import Hashtable

constant MUTE_ERROR_DURATION = 60
constant MUTE_ERROR_DURATION = 5

This comment has been minimized.

Sign in to view
Copy link
@Frotty

Frotty Jan 29, 2018

Member

I don't think this is a good idea. Should probably make the value @configurable though
@SamuelMoriarty
Revert mute duration change
Loading status checks…
d441184
@Frotty

This comment has been minimized.

Sign in to view
Copy link
Member

Frotty commented Jan 29, 2018

thanks a lot 🎓

@Frotty Frotty merged commit 894148c into wurstscript:master Jan 29, 2018
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SamuelMoriarty SamuelMoriarty deleted the SamuelMoriarty:persistence branch Mar 3, 2018

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