Sigma88 / TTSCarcassonne

Script and data for the Tabletop Simulator mod
MIT License
0 stars 0 forks source link

Ferries ToDo List #1

Closed Sigma88 closed 4 years ago

Sigma88 commented 5 years ago

from the rules:

Sigma88 commented 5 years ago

@DinnerBuffet I've been messing around on my fork of the mod

Now the deck should be complete, I had to add an extra state for the empty tile (with no ferry) because step 2 of the rules requires you to put down a follower before choosing a ferry, which means we need a way to put followers on the empty tile before connecting the roads.

I've also included the new save with the deck in the ferries sachel and automated the setup process.

Next steps will require me to learn a bit more about TTS API.

I pinged you just to keep you up-to-date, I know you are busy so I don't expect you to do anything.

DinnerBuffet commented 5 years ago

Cool, this is great progress! I can't think of anything missing from that checklist. I should get the chance this weekend to give some advice on which parts of the code need to be changed, but again busy :)

Sigma88 commented 5 years ago

I should get the chance this weekend to give some advice on which parts of the code need to be changed, but again busy :)

Don't worry, real life comes first and I might get busy as well very soon.

Sigma88 commented 5 years ago

so... I am abandoning the idea of using states, they are causing a lot of problems and it's probably not worth the work

I have been toying around with a new idea.

instead of storing the "linkedQuadrants" table in the tile itself, I am trying to store in a general table

this way, the lake tiles start off with no connections, and then I prompt the player to choose which 2 streets to connect (I will probably use a button for that)

the "linkedQuadrants" table will be stored in the general table and we can use the card's own guid to retrieve it

for now I managed to replace the loading by defining a new function:

function getLinkedQuadrants(tile)
    local t = FERRIESEXPANSION_LINKEDQUADRANTS[tile.guid]
    if t ~= nil then
        return t
    end
    return tile.getTable('linkedQuadrants')
end

and using this function here and here

links = tile.getTable('linkedQuadrants') becomes links = getLinkedQuadrants(tile)

DinnerBuffet commented 5 years ago

from the rules:

There's a good reference for this in the Carcassonne Annotated Rules (CAR) called "Order of Play" on page 207. I tried to use this loosely as the basis for the stateMachine (which, by the way, isn't a true "state machine" since there is some additional state information stored separately, such as hasAlreadyTakenBuilderTile).

  • [ ] whenever the state of a lake tile is changed

    • [ ] if so, revert the change if the player does not have permission

Like I previously said, I think the only way to do this would be to listen for an onObjectSpawn() event. You could then check if the object is a tile and also check if it has a Lake special feature to be double sure. As for restoring the original state, I'm not too sure.

  • [ ] after tile and (optional) follower placement, check if the new tile has a lake

    • [ ] if so, allow the current player to change the state of the current lake tile
    • [ ] check that the state is valid (state 1 is not valid)

1) Add another state between 'check added tile features' and 'check added tile special figure extension' called 'check added tile feature - lake' to check if the tile added has a lake: https://github.com/DinnerBuffet/TTSCarcassonne/blob/master/scripts/state_machine.ttslua#L23 2) Add handling for that state similar to this: https://github.com/DinnerBuffet/TTSCarcassonne/blob/master/scripts/state_machine.ttslua#L276 3) If the tile doesn't have a Lake you can just call nextState('next'). If the tile has a Lake you'll want to create a prompt to confirm the object state. The function will look similar to other functions for checking if a feature is present on a tile. Pressing the confirm button should call removeTile and addTile, then nextState('next')

  • [ ] then, check if the new tile extended a road attached to a lake

    • [ ] if so, allow the current player to change the state of the closest affected lake tile
    • [ ] check that the state is valid (state 1 is not valid)

I gave this a bit more thought and I think you need to store additional information in the featureList about the location of the ferry, like the following: https://github.com/DinnerBuffet/TTSCarcassonne/blob/master/scripts/feature_map_management.ttslua#L331

Little bit of background, in an effort to make adding and removing tiles as efficient as possible (in order to make the AI quicker), I stored all of the necessary data in the featureList. By only storing the basic necessary information, AI calculations could be done super quick. For example, the only information stored about a road is numTiles, numOpenings, and a list of features associated with it (ie. Inns). When a tile is added to this road, we can simply increase the tiles to numTiles + 1 and the openings to numOpenings - 1 + number of openings on the road in the new tile. If the number of openings gets to 0, we know the road finished and we can score it for numTiles points. The featureMap then keeps a simple map to where each of these features is. I have a debugging tool that I use to view the featureList/featureMap data. I will try to add it to the workshop later, which should help.

This system of storing basic metadata worked so well that I eventually decided to use this method to replace my original tile traversal method for non-AI calculations as well. For some special cases, though, we need to know on which tile a feature exists (ie. cloisters). That is why the featureLocation property exists on some features.

Unfortunately, featureLocation is a property of the feature, which is in this case the feature is the road, and the ferry is a specialFeature of that road. Since the road could potentially have multiple ferries, I think it makes more sense for each ferry to be it's own feature in the featureList, and then to link them to the road. Currently the only way to link separate features together is with linkedSpecialFeatures (should really be called linkedFeatures I think), which I will get to in a bit...

so... I am abandoning the idea of using states, they are causing a lot of problems and it's probably not worth the work

I have been toying around with a new idea.

instead of storing the "linkedQuadrants" table in the tile itself, I am trying to store in a general table

this way, the lake tiles start off with no connections, and then I prompt the player to choose which 2 streets to connect (I will probably use a button for that)

the "linkedQuadrants" table will be stored in the general table and we can use the card's own guid to retrieve it

for now I managed to replace the loading by defining a new function:

function getLinkedQuadrants(tile)
    local t = FERRIESEXPANSION_LINKEDQUADRANTS[tile.guid]
    if t ~= nil then
        return t
    end
    return tile.getTable('linkedQuadrants')
end

and using this function here and here

links = tile.getTable('linkedQuadrants') becomes links = getLinkedQuadrants(tile)

What you could do is link the roads together with linkedSpecialFeatures similar to Road Intersection. This way the roads could remain separate, and changing the ferries would only really need to change the linkedSpecialFeatures. However, it would require a lot of other special handling such as the scoring, so using the hack you proposed to make the roads the same feature in addTile might be easier in the long-run. You might still need to keep a linkedSpecialFeature to tie the roads to the lakes, though.

If you get confused it's probably not your fault. The code is a bit of a hacky mess. Again, I'm sorry that I can't be more help right now. I'm getting married this week and I'm crazy busy.

Sigma88 commented 5 years ago

If you get confused it's probably not your fault. The code is a bit of a hacky mess. Again, I'm sorry that I can't be more help right now. I'm getting married this week and I'm crazy busy.

haha, have fun! and don't worry about me, I'm posting here more to keep track of my progress than to get your attention :)

Sigma88 commented 5 years ago

ok, as far as my tests go I think I got it to work

here's a short video I made:

it still needs some polish (chat messages and stuff like that) but all the mechanics are there

DinnerBuffet commented 5 years ago

That looks great! Well done! How did you end up coding everything? Today I will make some time to upload the debugger tool, which should help you figure out any odd issues.

DinnerBuffet commented 5 years ago

Here it is: https://steamcommunity.com/sharedfiles/filedetails/?id=1760133638

Hope you find this helpful. Hopefully I should find time soon to be of more help.

Sigma88 commented 5 years ago

That looks great! Well done! How did you end up coding everything?

I've added some new states to the state machine

I've written a function that generates some buttons over the lake tile to place or move the ferry, and also a function that looks for the nearest lake whenever you extend a street

whenever a ferry needs to be moved, I just remove the tile and replace it on the same spot (idk if that's bad, it doesn't seem to do any damage)

and to keep track of which street connects to what I wrote a bypass function to provide the linkedQuadrants from a separate table

DinnerBuffet commented 5 years ago

That looks great! Well done! How did you end up coding everything?

I've added some new states to the state machine

I've written a function that generates some buttons over the lake tile to place or move the ferry, and also a function that looks for the nearest lake whenever you extend a street

From my quick check I don't see any major issues with the way it was coded. Great work! You'll want to double-check that special tiles such as '676a91' from Abbey & Mayor, '094980' from Traders & Builders, and '6a7647' from Inns & Cathedrals don't break it. I think that they should work, though.

whenever a ferry needs to be moved, I just remove the tile and replace it on the same spot (idk if that's bad, it doesn't seem to do any damage)

For these circumstances it is perfectly fine. If you needed to do it hundreds of times per move (ie. AI calculations) then it wouldn't be ideal, but I don't think a very complicated AI for this expansion should be the first thing on the todo list.

and to keep track of which street connects to what I wrote a bypass function to provide the linkedQuadrants from a separate table

Normally I try to avoid any specialized data embedded in the tiles whenever I can, since changing it later can be a nuisance. As long as it doesn't break what's already in the mod I'm fine with the hack.

I noticed that you sent a pull request. Once again I am busy so it will take me sometime before I'll be able to go over it in detail. Please don't take this as a sign that your work isn't appreciated because it certainly is!

Sigma88 commented 5 years ago

If I sent a pull request it was by mistake.

I don't think this is ready to be merged yet

Don't worry about that, I will close it

Edit: I checked and there are no pull requests on your repo You probably got confused with the PR I push on the master branch of my fork

I do that everytime I finish adding a big feature so that I can work on my "development" branch without worrying to mess up too much

DinnerBuffet commented 5 years ago

I checked and there are no pull requests on your repo You probably got confused with the PR I push on the master branch of my fork

I do that everytime I finish adding a big feature so that I can work on my "development" branch without worrying to mess up too much

Yup I think you were right.

How are things? My availability is opening up and I can help implement/give advice if needed.

Sigma88 commented 5 years ago

How are things? My availability is opening up and I can help implement/give advice if needed.

I have been on pause for a couple of weeks because I was a bit busy.

The changes are mostly finished, all mechanics work as far as I could test.

I would like to improve a bit the notifications when a ferry event is triggered, and I would like to make sure the scoring features work fine.

I had some issues with scoring but I discovered that's a bug present in the base mod as well so I don't think it was triggered by my changes. (when you go back in time ctrl-z sometimes you end up breaking the scoring at the end) I also got a NRE after clicking on the score summary button but I wasn't able to understand if it was an issue with the base mod or with my modification

EDIT: hope everything is fine with you :D

DinnerBuffet commented 5 years ago

How are things? My availability is opening up and I can help implement/give advice if needed.

I have been on pause for a couple of weeks because I was a bit busy.

The changes are mostly finished, all mechanics work as far as I could test.

I would like to improve a bit the notifications when a ferry event is triggered, and I would like to make sure the scoring features work fine.

I had some issues with scoring but I discovered that's a bug present in the base mod as well so I don't think it was triggered by my changes. (when you go back in time ctrl-z sometimes you end up breaking the scoring at the end) I also got a NRE after clicking on the score summary button but I wasn't able to understand if it was an issue with the base mod or with my modification

Sorry about that. I broke it in 1.21.11 and fixed it in 1.21.13

I'll try out your changes soon and see if I can break it.

One thing I want to point out is that versioning the JSON file didn't seem to work well at all. The commit was a mess and included micro-changes in the positions of objects that weren't even touched. Merging probably won't work well. I'm fairly sure that I haven't made any recent non-code changes. If you had, you can simply use your JSON.

EDIT: hope everything is fine with you :D

Things are great, thanks!

Sigma88 commented 5 years ago

One thing I want to point out is that versioning the JSON file didn't seem to work well at all. The commit was a mess and included micro-changes in the positions of objects that weren't even touched. Merging probably won't work well. I'm fairly sure that I haven't made any recent non-code changes. If you had, you can simply use your JSON.

every time I push to the file I tried to make sure that only meaningfull changes were included, but some indenting change might have get into it, I would have to check

DinnerBuffet commented 5 years ago

One thing I want to point out is that versioning the JSON file didn't seem to work well at all. The commit was a mess and included micro-changes in the positions of objects that weren't even touched. Merging probably won't work well. I'm fairly sure that I haven't made any recent non-code changes. If you had, you can simply use your JSON.

every time I push to the file I tried to make sure that only meaningfull changes were included, but some indenting change might have get into it, I would have to check

I didn't put any such effort into my changes. Sorry :)

DinnerBuffet commented 5 years ago

How are things? Sorry for the long delay. My life has been quite busy. I finally got the time to give the changes a try and they worked pretty well, but I was able to find a few bugs once the ferries changed to a new orientation. If you have any issue reproducing I could give you a save file.

I had a more close look at the code and I think the solution itself is pretty solid. I think the getLinkedQuadrants hack is actually a pretty good solution to the problem, and I like that the linkedQuadrants for each ferries tile are saved. This is much simpler than what I probably would have done. I could only really go through your latest commit, though. I tried to squash all of the commits and go through them, but the random white space changes made it really hard to find any details I might have missed. One thing I didn't really understand was why activeTiles was necessary, since you could just get the location of the tile real-time.

There were also a few stylistic things that I could suggest changes to, but I think the overall implementation should work. This is, of course, without knowing what is the cause of the previously mentioned bugs.

Sigma88 commented 5 years ago

Hey! I've been pretty busy myself so don't worry...

Can you describe the bug(s) you found?

The bug I found happens when using the "undo" option. After using "undo" even just once during the fame, at the "end game/score points" step the game returns an error. I havent managed to pinpoint that one yet

DinnerBuffet commented 5 years ago

It actually was a couple weeks ago so I'll have to give it another try and let you know. If I remember correctly it was after changing the orientation of a couple of ferries linked together. The ferries all change orientation properly, but then afterwards some uncompleted, open roads are scored immediately. Will get back to you with more details.

The bug you mentioned after using undo was in my code and was fixed in 1.21.13.

DinnerBuffet commented 5 years ago

I gave it another try tonight and I believe that the previous "bugs" might have just been me misunderstanding the rules. I did my best but was unable to break it :)

Do you think you would find the time to clean up the commits into something manageable? If you can, then I can give some more feedback and we can work on pulling in the code.

Full disclaimer: I'm not too familiar with collaborating on github, so let me know if you have a more efficient process.

Sigma88 commented 5 years ago

sure I'll have to fix a couple of changes and then I will send you a pull request that should have only the meaningful changes

DinnerBuffet commented 4 years ago

Hi there, I finally put some time into this and have what I consider to be a finished version. I manually squashed and merged the code over my main branch (not yet pushed), leaving out a bunch of the white-space changes that were done (not that I don't agree with them, I just want to keep the commit concise to these changes without too much refactoring). In addition to some general cleanup, I changed the following things:

  1. I like to have all buttons rotate towards the seated position of the player, so that no player is forced to rotate their camera to read the buttons.
  2. Buttons attached to objects share that object's scale. Because TTS cards need to be warped to be a square tile, this makes the buttons warped. Using createNormalizedArbitraryButton attaches them to a hidden object with a fixed scale.

Is there anything else you intended to fix up before I upload this? How do you prefer to be credited? Is Sigma88 fine? Also, can you give me a link to your steam profile so that I can add you as an author?

Thanks again for the contribution!

Sigma88 commented 4 years ago

hi there, sorry for disappearing but RL got in the way 🤣

I'll reply to your questions as much as I can:

Hi there, I finally put some time into this and have what I consider to be a finished version. I manually squashed and merged the code over my main branch (not yet pushed), leaving out a bunch of the white-space changes that were done (not that I don't agree with them, I just want to keep the commit concise to these changes without too much refactoring).

most whitespace changes were probably introduced by my text editor, I have no objection to them being reverted, however there are a couple of spaces that I changed in the list of expansions checkboxes to make all of them consistent, some had a space between the number and the dot, some didn't have that space. so you might want to check those to see if the inconsistencies are still there

In addition to some general cleanup, I changed the following things:

  • removed activeTiles, since it wasn't necessary to store this information in the save file.

I don't really remember why I added those, however I think it's probably because of a lack in understanding of how the mod works, if everything is fine without them it makes it cleaner to remove them

changed ferry buttons and labels to use createNormalizedArbitraryButton. This is for two reasons:

  • I like to have all buttons rotate towards the seated position of the player, so that no player is forced to rotate their camera to read the buttons.
  • Buttons attached to objects share that object's scale. Because TTS cards need to be warped to be a square tile, this makes the buttons warped. Using createNormalizedArbitraryButton attaches them to a hidden object with a fixed scale.

again, probably a result of my lack of knowledge, I just tried to get the mod to work and didn't really care about the aestetics, I'm glad you found a more pleasing solution

Is there anything else you intended to fix up before I upload this?

Playing with the base version of the mod, I sometimes wish there was a confirmation button after placing a tile or a follower. I don't know how hard this would be to implement, but it would save a lot of grief from dropping followers on the wrong spot and get credited with an empty field rather than a street or city. This issue is most common with followers but occasionally it happened that I dropped a tile and immediately regretted it. So this might be an idea for a future development.

How do you prefer to be credited? Is Sigma88 fine? Also, can you give me a link to your steam profile so that I can add you as an author? Thanks again for the contribution!

I don't really care about getting credit for the few changes I made, expecially since you had to go through much of the code and fix it yourself. If you want to credit me as "Sigma88" it is fine but it's up to you really.

Thanks for developing this mod, it's definitely the one I use the most with TTS

DinnerBuffet commented 4 years ago

most whitespace changes were probably introduced by my text editor, I have no objection to them being reverted, however there are a couple of spaces that I changed in the list of expansions checkboxes to make all of them consistent, some had a space between the number and the dot, some didn't have that space. so you might want to check those to see if the inconsistencies are still there

You must mean this? https://github.com/Sigma88/TTSCarcassonne/commit/1bc71a94c7e91176829dd25b5c793acd3d4934cf

If so you're right, I hadn't noticed. I appreciate your attention to detail :) Is there anything else that wasn't obvious? I pushed the mostly-unchanged ported code here: https://github.com/DinnerBuffet/TTSCarcassonne/commit/100f654c63c128848348546a80b5148cde7a6324

I will push my changes once they are final.

I don't really remember why I added those, however I think it's probably because of a lack in understanding of how the mod works, if everything is fine without them it makes it cleaner to remove them

My general rule is that is that if any information could be lost by saving/loading during a timer delay, coroutine sleep, or while waiting for user input, then that data must be saved. In this case, the data saved in activeTiles (x + y of the tile) is available after the user pushes the button, so it can be stored in a global to be used by the button callback.

again, probably a result of my lack of knowledge, I just tried to get the mod to work and didn't really care about the aestetics, I'm glad you found a more pleasing solution

It probably didn't help that I misused the word "normalized". I'm still not entirely sure what I meant by that...

Playing with the base version of the mod, I sometimes wish there was a confirmation button after placing a tile or a follower. I don't know how hard this would be to implement, but it would save a lot of grief from dropping followers on the wrong spot and get credited with an empty field rather than a street or city. This issue is most common with followers but occasionally it happened that I dropped a tile and immediately regretted it. So this might be an idea for a future development.

I don't advertise this well, but anytime during a turn, the tile that was placed can be unlocked and lifted to reset the turn back to the tile placement phase. As for placed followers, their positions are evaluated real-time, so players always have the option to pick them up and move them to where they intended after the fact. Here it's arguable, though, to have a confirmation in case points are scored by misplacing it, but you can always subtract the points that were scored using the score counters. I prefer to keep the flow moving forward and I try to make a long enough delay so that players can pick the tile/follower up again if they misplace it.

I don't really care about getting credit for the few changes I made, expecially since you had to go through much of the code and fix it yourself. If you want to credit me as "Sigma88" it is fine but it's up to you really.

You absolutely deserve credit. Not only did you have to learn all of the silly nuances of my code, but this was a non-trivial change and you wrote all of the code without any real help. There's a reason you're the only contributor in the years that this mod has been in development. Kudos to you for sure!

Sigma88 commented 4 years ago

You must mean this? 1bc71a9

yes 👍

Is there anything else that wasn't obvious?

difficult to tell, I found some issues in the rivers II tiles: one of them is missing a "shield" (or whatever it's called) from a city

there might be other stuff, but honestly I don't remember right now, if I come across some more issues I will make sure to post them on your repo :)

everything else looks good to me

DinnerBuffet commented 4 years ago

difficult to tell, I found some issues in the rivers II tiles: one of them is missing a "shield" (or whatever it's called) from a city

Wow you're right! I rarely find tile data issues anymore. Kudos!

I'll upload a new revision to the workshop. Thanks again for the help!

Sigma88 commented 4 years ago

Congrats on the release

Closing this issue since the changes have been included in the latest release

:tada: