Fix issue #106 - #118
Conversation
|
how does this approach the fix? |
|
Its the redstone that's the issue. I dont think this will fix anything. |
|
@ttk2 It checks the blocks on all 4 sides of a pressure plate for openable blocks. If a reinforcement is found and the player doesn't have access to it the event is cancelled preventing access. |
|
Tested it, this does fix the issue #106. |
|
Can one of the admins verify this patch? Type 'ok to test' to test. |
|
My only real concern with the patch is since it's on physical interaction, every player physical interaction will trigger this event (potentially). I haven't tested to see what in all triggers it, so hopefully someone can/will/has and assuage my fears, but this could potentially be a bit of a resource hog if such a check is run too often. |
|
So would it be better to approach this as a redstone interaction as @rourke750 mentioned? |
|
Can't speak to that, but it would be better to check that the block being interacted with is of the class of interact-able blocks where this fix matters, before checking all the sides. E.g. fail fast, and reduce running time within the method to the minimal set of tests. Make sure that the test likely to discard the largest number of interactions happens first, etc. |
|
I am working on an alternate implementation. |
|
Maybe something like this: |
|
Significant improvement. Drop the stack frame cost of the function call for the press-able check unless you're using it in multiple places. Edit: In fact both checks should simply live inside the IF statement, unless we've got motivation to use them elsewhere it's a premature optimization, and introduces an extra frame/context switch cost per execution that we don't need. |
|
I was thinking the same, but left it there as i was testing for clarity of the gist. I could make a pull for it. |
|
Sounds good! |
|
@suirad @ProgrammerDan Would you like me to update this pull request with the changes you have suggested? |
|
That'd be awesome! |
There was a problem hiding this comment.
instanceof will return false if the object is null, so you dont need an additional null check here
|
@TreeDB Your change looks good. |
|
Seconded, much improved even with just those small changes. Should be good to go; have you tested it locally, @TreeDB ? |
|
ok to test |
|
drat, can't get a build to test it with. @ProgrammerDan think I should merge and throw on Civtest? |
|
ok to test |
|
@ttk2 lol let me look into that. |
|
I think you just did something weird. |
|
did you put the testing build on Civtest? |
|
I did not, citadel is still broken from the other changes. I'll try getting to this later in the week. I might have time later today to fix citadel. |
|
ok to test |
|
@Maxopoly @rourke750 any reason you can think of not to include this? Has it been superceded by future changes? |
|
I forgot this existed. If it works might as well merge. I dont believe this was fixed anywhere else. |
|
Ok, going to do an omnibus PR w/ database change to new CivModCore standard, I'll tack this on. Will test it then too. |
7 participants
No description provided.