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

[RDY] Door alignments with TH #1163

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mugmuggy
Contributor

mugmuggy commented Nov 20, 2016

screenToWall - Swing door placements in corners of walls matches original TH which - north wall, west corner is 2 tiles over for example

door_floor_blueprint_markers - aligns with TH

checkDoorWalls - update to return an invalid_tile flag to identify the door blueprints that are invalid.

validDoorTile - removed the flag name checks. Does further additional checks for passable tiles in the walkable tiles zone of objects, except bench. Stops drinks machine and reception desk being placed in front of an door for example.

setDoorBlueprint - created a local function - doorWallOffsetCalculations as the functionality needs to be called on previous tile, aswell as current tile position as swing door floor blueprints take 3 tiles, mapSetCell had to be inserted intothe for loop, the rest of the updates related to the invalid door tile changes

mugmuggy added some commits Nov 20, 2016

@mugmuggy

This comment has been minimized.

Show comment
Hide comment
@mugmuggy

mugmuggy Dec 1, 2016

Contributor

Seeking more feedback on this as I've combined 1164 changes here as they use the same refactored function now and that bit of code is more less complicated than the greater change

Contributor

mugmuggy commented Dec 1, 2016

Seeking more feedback on this as I've combined 1164 changes here as they use the same refactored function now and that bit of code is more less complicated than the greater change

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Dec 1, 2016

Member

You seem to have ended up with one of @Alberth289346's commits in your PR. It shouldn't be there. Do you know enough about git to remove it cleanly?

I'm not a big fan of using a bit mask for invalid_tile. Bit manipulation doesn't play to lua's strengths, a table seems more appropriate. At the very least it should be documented (I didn't see any documentation on it / what the bit positions mean) in your PR.

Member

TheCycoONE commented Dec 1, 2016

You seem to have ended up with one of @Alberth289346's commits in your PR. It shouldn't be there. Do you know enough about git to remove it cleanly?

I'm not a big fan of using a bit mask for invalid_tile. Bit manipulation doesn't play to lua's strengths, a table seems more appropriate. At the very least it should be documented (I didn't see any documentation on it / what the bit positions mean) in your PR.

Show outdated Hide outdated CorsixTH/Lua/dialogs/edit_room.lua
Show outdated Hide outdated CorsixTH/Lua/dialogs/edit_room.lua
-- If we're dealing with swing doors the anim variable is actually a table with three
-- identical "doors".
for i, anim in ipairs(self.blueprint_door.anim) do
anim:setAnimation(self.anims, self.blueprint_door.old_anim[i],
self.blueprint_door.old_flags[i])
anim:setTag(nil)
self.blueprint_door.anim[i] = nil
oldx = oldx_mod and self.blueprint_door.floor_x + (i - oldx_mod) or self.blueprint_door.floor_x

This comment has been minimized.

@TheCycoONE

TheCycoONE Dec 1, 2016

Member

Why did you put this inside the anim loop? It looks like it would do the same thing every loop iteration unless I'm missing something.

@TheCycoONE

TheCycoONE Dec 1, 2016

Member

Why did you put this inside the anim loop? It looks like it would do the same thing every loop iteration unless I'm missing something.

This comment has been minimized.

@mugmuggy

mugmuggy Oct 1, 2017

Contributor

Just to cover this off - I've updated the code to support the 3 tile blueprint for swing doors.

The doorWallOffsetCalculations function returns nil for oldx_mod/oldy_mod depending on orientation and the (i -oldx(y)_mod) calculation performed within.

The old code would end that loop and then call
map:setCell(self.blueprint_door.floor_x, self.blueprint_door.floor_y, 4, 24) which would be the centre tile of the door.

But that is just one blueprint tile being reset. So upon adding the support for the blueprint for the entire 3 tiles of the swing door, all 3 tiles need to be cleaned up, and the supporting calculation is done on the orientation.

@mugmuggy

mugmuggy Oct 1, 2017

Contributor

Just to cover this off - I've updated the code to support the 3 tile blueprint for swing doors.

The doorWallOffsetCalculations function returns nil for oldx_mod/oldy_mod depending on orientation and the (i -oldx(y)_mod) calculation performed within.

The old code would end that loop and then call
map:setCell(self.blueprint_door.floor_x, self.blueprint_door.floor_y, 4, 24) which would be the centre tile of the door.

But that is just one blueprint tile being reset. So upon adding the support for the blueprint for the entire 3 tiles of the swing door, all 3 tiles need to be cleaned up, and the supporting calculation is done on the orientation.

Show outdated Hide outdated CorsixTH/Lua/dialogs/edit_room.lua
Show outdated Hide outdated CorsixTH/Lua/dialogs/edit_room.lua
Updated utility function, added refactored method to world, updated e…
…dit_room/place objects to use the updated method
@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Sep 29, 2017

Member

This condition will only exit if the tile is not buildable and the player doesn't own it. I think you meant that or to be an and to match the comment above.
If it's simpler to understand, rewrite as if not tile_flags.buildable or tile_flags.owner ~= player_id

Member

TheCycoONE commented on CorsixTH/Lua/dialogs/edit_room.lua in 1b06b94 Sep 29, 2017

This condition will only exit if the tile is not buildable and the player doesn't own it. I think you meant that or to be an and to match the comment above.
If it's simpler to understand, rewrite as if not tile_flags.buildable or tile_flags.owner ~= player_id

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Sep 29, 2017

Member

Should be 4 spaces for a line continuation

Member

TheCycoONE commented on CorsixTH/Lua/dialogs/edit_room.lua in 1b06b94 Sep 29, 2017

Should be 4 spaces for a line continuation

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Sep 29, 2017

Member

I am largely happy with the direction of the latest attempt. I think the flag name passthrough could be clearer, I had to read your comment to know what it meant. Maybe sharable or exclusive?

Member

TheCycoONE commented Sep 29, 2017

I am largely happy with the direction of the latest attempt. I think the flag name passthrough could be clearer, I had to read your comment to know what it meant. Maybe sharable or exclusive?

Updates based on comments
Utility - updated with support for builtins form Lua 5.3/5.2 or workaround fors 5.1
Cleaned up some conditions/checks for readability or that were redundant
@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 3, 2017

Member

That luadoc error isn't very helpful. I'll try to get to the bottom of it.

Member

TheCycoONE commented Oct 3, 2017

That luadoc error isn't very helpful. I'll try to get to the bottom of it.

@TheCycoONE

ldocgen issues

Show outdated Hide outdated CorsixTH/Lua/utility.lua
Show outdated Hide outdated CorsixTH/Lua/world.lua
@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 10, 2017

Member

I did a bit of testing, and I'm confused by the following:

Why is no door allowed here:
no-door

And a door allowed here?
yes-door

My understanding (though I haven't double checked the original) is that the bench footprint shouldn't block the door in either case.

Member

TheCycoONE commented Oct 10, 2017

I did a bit of testing, and I'm confused by the following:

Why is no door allowed here:
no-door

And a door allowed here?
yes-door

My understanding (though I haven't double checked the original) is that the bench footprint shouldn't block the door in either case.

@MarkL1961

This comment has been minimized.

Show comment
Hide comment
@MarkL1961

MarkL1961 Oct 10, 2017

Contributor

@TheCycoONE I would have said that it should not be allowed in either case, unless the bench was rotated. This is purely from memory though, as the legs of those waiting would block access to the door. If I can remember how to run TH in dosbox I will check this out.

Having now checked TH both of these are allowed

option a
option b

Contributor

MarkL1961 commented Oct 10, 2017

@TheCycoONE I would have said that it should not be allowed in either case, unless the bench was rotated. This is purely from memory though, as the legs of those waiting would block access to the door. If I can remember how to run TH in dosbox I will check this out.

Having now checked TH both of these are allowed

option a
option b

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 10, 2017

Member

Thanks for checking @MarkL1961
I think that's why @mugmuggy had to special case benches - because the same sort of situation isn't allowed with drink machines?

Anyway it seems to not always be working.

Member

TheCycoONE commented Oct 10, 2017

Thanks for checking @MarkL1961
I think that's why @mugmuggy had to special case benches - because the same sort of situation isn't allowed with drink machines?

Anyway it seems to not always be working.

@mugmuggy

This comment has been minimized.

Show comment
Hide comment
@mugmuggy

mugmuggy Oct 10, 2017

Contributor

Only thing I can think of is fa38fca was missed but then you couldn't build the one next to the radiator either if that was the case.

Contributor

mugmuggy commented Oct 10, 2017

Only thing I can think of is fa38fca was missed but then you couldn't build the one next to the radiator either if that was the case.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 11, 2017

Member

I built the first bench before loading your branch, and the second after - perhaps existing benches aren't getting the new flag?

Member

TheCycoONE commented Oct 11, 2017

I built the first bench before loading your branch, and the second after - perhaps existing benches aren't getting the new flag?

Added afterLoad checks and included Chair from GPs Office as it requi…
…res footprint change to be placed next to door

@mugmuggy mugmuggy changed the title from [RFC] Door alignments with TH to [RDY] Door alignments with TH Oct 23, 2017

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 3, 2017

Member

The savegame version must be set. It also must be bumped as 118 was taken by the mouse capture pr. I'd like to land this next if you could focus here @mugmuggy?

Member

TheCycoONE commented Nov 3, 2017

The savegame version must be set. It also must be bumped as 118 was taken by the mouse capture pr. I'd like to land this next if you could focus here @mugmuggy?

mugmuggy added some commits Nov 3, 2017

TheCycoONE added a commit to TheCycoONE/CorsixTH that referenced this pull request Nov 10, 2017

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 10, 2017

Member

Merged

Member

TheCycoONE commented Nov 10, 2017

Merged

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