search

Civcraft / Citadel

Do not open issues here; open them on the maintained fork @ DevotedMC
https://github.com/DevotedMC/Citadel
BSD 3-Clause "New" or "Revised" License
6 stars 23 forks source link

Fix issue #106 #118

Open TreeDB opened 8 years ago

ttk2 commented 8 years ago

how does this approach the fix?

rourke750 commented 8 years ago

Its the redstone that's the issue. I dont think this will fix anything.

TreeDB commented 8 years ago

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

suirad commented 8 years ago

Tested it, this does fix the issue #106.

CivcraftBot commented 8 years ago

Can one of the admins verify this patch? Type 'ok to test' to test.

ProgrammerDan commented 8 years ago

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.

suirad commented 8 years ago

So would it be better to approach this as a redstone interaction as @rourke750 mentioned?

ProgrammerDan commented 8 years ago

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.

suirad commented 8 years ago

I am working on an alternate implementation.

suirad commented 8 years ago

Maybe something like this: https://gist.github.com/suirad/6b8330c824fd999d3560

ProgrammerDan commented 8 years ago

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.

suirad commented 8 years ago

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.

ProgrammerDan commented 8 years ago

Sounds good!

TreeDB commented 8 years ago

@suirad @ProgrammerDan Would you like me to update this pull request with the changes you have suggested?

ProgrammerDan commented 8 years ago

That'd be awesome!

suirad commented 8 years ago

@TreeDB Your change looks good.

ProgrammerDan commented 8 years ago

Seconded, much improved even with just those small changes. Should be good to go; have you tested it locally, @TreeDB ?

TreeDB commented 8 years ago

@ProgrammerDan No

ttk2 commented 8 years ago

ok to test

ttk2 commented 8 years ago

drat, can't get a build to test it with. @ProgrammerDan think I should merge and throw on Civtest?

rourke750 commented 8 years ago

ok to test

rourke750 commented 8 years ago

@ttk2 lol let me look into that.

rourke750 commented 8 years ago

I think you just did something weird.

ttk2 commented 8 years ago

did you put the testing build on Civtest?

rourke750 commented 8 years ago

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.

rourke750 commented 8 years ago

ok to test

ProgrammerDan commented 8 years ago

@Maxopoly @rourke750 any reason you can think of not to include this? Has it been superceded by future changes?

rourke750 commented 8 years ago

I forgot this existed. If it works might as well merge. I dont believe this was fixed anywhere else.

ProgrammerDan commented 8 years ago

Ok, going to do an omnibus PR w/ database change to new CivModCore standard, I'll tack this on. Will test it then too.