Skip to content
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.

Sign up for GitHub

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

[FEATURE] control initial number of mages drawn #380

Closed
wants to merge 1 commit into from
Closed

[FEATURE] control initial number of mages drawn #380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .vs/VSWorkspaceState.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
Copy link
Owner

@on3iro on3iro Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove these vs-code files :) It would be awesome, if you could add a .gitignore part for vscode. You can probably find something useful here https://github.com/github/gitignore

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same goes for the .vs/slnx.sqlite file)

"ExpandedNodes": [""],
"SelectedNode": "\\src\\Redux\\Store\\Expeditions\\Expeditions\\helpers\\convertExpeditionToConfig.ts",
"PreviewInSolutionExplorer": false
}
Binary file added .vs/slnx.sqlite
View file
Open in desktop
Binary file not shown.
31 changes: 24 additions & 7 deletions src/Redux/Store/Expeditions/Expeditions/helpers/convertExpeditionFromConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import * as types from 'aer-types'
import shortid from 'shortid'
import { selectors } from 'Redux/Store'
import { getMageIdsFromSimpleArray } from 'Redux/Store/Expeditions/Expeditions/sideEffects/helpers'
import { determineUsedExpansions } from 'Redux/Store/Expeditions/Expeditions/sideEffects/createSettingsSnapshot/determineUsedExpansions'
import { RootState } from 'Redux/Store'
import { getLatestMigrationVersion } from 'Redux/Store/Expeditions/Expeditions/migrations'
Expand All @@ -9,17 +11,32 @@ export const convertExpeditionFromConfig = (
state: RootState
): types.Expedition => {
const expeditionId = shortid.generate()

const seed = {
seed: config.seedConfig || shortid.generate(),
supplyState: true,
nemesisState: true,
}
const allMages = selectors.Settings.Expansions.Mages.content.getContent(state)
.ENG
const mageArray = Object.keys(allMages).map(function (id) {
let mage = allMages[id]
return mage
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just return allMages[id] here instead of making and intermediate assignment :)

})
if (config.initialBarracksConfig) {
config.initialBarracksConfig.mageIds = getMageIdsFromSimpleArray({
mage: config.initialBarracksConfig.mageIds,
seed,
stillAvailableMageIds: mageArray.map(({ id }) => id),
}).result
}
console.log(allMages)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the console.logs please :)

console.log(mageArray.map(({ id }) => id))
const expedition = {
id: expeditionId,
name: config.name,
bigPocketVariant: config.bigPocketVariantConfig,
score: 0,
seed: {
seed: config.seedConfig || shortid.generate(),
supplyState: true,
nemesisState: true,
},
seed: seed,
barracks: {
supplyIds: config.initialBarracksConfig?.supplyIds || [],
mageIds: config.initialBarracksConfig?.mageIds || [],
Expand All @@ -40,7 +57,7 @@ export const convertExpeditionFromConfig = (
sequence: {
firstBranchId: config.sequenceConfig.firstBranchId,
branches: Object.keys(config.sequenceConfig.branches)
.map(key => {
.map((key) => {
const branch = {
...config.sequenceConfig.branches[key],
id: key,
Expand Down
64 changes: 57 additions & 7 deletions src/Redux/Store/Expeditions/Expeditions/sideEffects/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export const getSupplyIds = ({
// 1.) it lets us filter available cards by those ids, which have been explicitely specified
// 2.) it makes the reduce call a bit easer to read and maintain, as we don't have to handle the 'string' case
const ids = supply.ids.filter(
idOrBlueprint => typeof idOrBlueprint === 'string'
(idOrBlueprint) => typeof idOrBlueprint === 'string'
)
const bluePrints = supply.ids.filter(
(idOrBlueprint): idOrBlueprint is types.IBluePrint =>
Expand All @@ -77,7 +77,7 @@ export const getSupplyIds = ({
(acc: RewardResult, blueprint) => {
const cardType = blueprint.type
const stillAvailableCards = stillAvailableCardsByType[cardType].filter(
card => ids.indexOf(card.id) === -1
(card) => ids.indexOf(card.id) === -1
)

const cardCreationResult = createCardList(
Expand All @@ -90,7 +90,7 @@ export const getSupplyIds = ({
return {
result: [
...acc.result,
...cardCreationResult.result.map(card => card.id),
...cardCreationResult.result.map((card) => card.id),
],
seed: cardCreationResult.seed,
}
Expand Down Expand Up @@ -125,7 +125,7 @@ export const getTreasureIds = ({
return baseResult
} else {
const ids = treasure.ids.filter(
idOrRandom => typeof idOrRandom === 'string'
(idOrRandom) => typeof idOrRandom === 'string'
)

const randomTreasureConfigs = treasure.ids.filter(
Expand All @@ -138,7 +138,7 @@ export const getTreasureIds = ({
const treasureLevel = config.level
const stillAvailableCards = stillAvailableTreasureIdsByLevel[
treasureLevel
].filter(treasureId => ids.indexOf(treasureId) === -1)
].filter((treasureId) => ids.indexOf(treasureId) === -1)

const treasureIdsResult = createIdList(
stillAvailableCards,
Expand Down Expand Up @@ -177,7 +177,7 @@ export const getMageIds = ({
if (mage === undefined) {
return baseResult
} else {
const ids = mage.ids.filter(idOrRandom => typeof idOrRandom === 'string')
const ids = mage.ids.filter((idOrRandom) => typeof idOrRandom === 'string')

const randomTreasureConfigs = mage.ids.filter(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should definitely change the naming here - this is probably a copy and paste issue (i copied some internals from the treasure generation), but its very misleading. -> in turn we should also change it in your code as well.

(idOrRandom): idOrRandom is { random: true } =>
Expand All @@ -187,7 +187,57 @@ export const getMageIds = ({
return randomTreasureConfigs.reduce(
(acc: RewardResult, _) => {
const filteredMageIds = stillAvailableMageIds.filter(
mageId => ids.indexOf(mageId) === -1
(mageId) => ids.indexOf(mageId) === -1
)

const mageIdsResult = createIdList(
filteredMageIds,
['EMPTY'],
undefined,
acc.seed
)

return {
result: [...acc.result, ...mageIdsResult.result],
seed: mageIdsResult.seed,
}
},
{
...baseResult,
result: ids,
} as RewardResult // FIXME casting should technically not be necessary (but i could make it work without)
)
}
}

export const getMageIdsFromSimpleArray = ({
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this function seems to be almost identical to getMageIds() above. I think we should remove this additional function and use getMageIds() instead.

We could probably type our initialBaracksConfig() in a way, that uses our various rewards config types (e.g. MageRewardsConfig - that way it would be compatible without us doing much.

The only thing we then will have to do, is to write a new migration.

Copy link
Author

@Quicksav Quicksav Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had thought about this. I wasn't sure if thats the route we wanted to go because technically its not really a reward, and the rewards config is just a slightly more complex object compared to what I have. But I also see the merit of not having to keep both functions up to date going forward.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - I think an additional benefit of this would be the consistency for users writing these configs.

mage,
stillAvailableMageIds,
seed,
}: {
mage: Array<string | { random: true }> | undefined
seed: types.Seed
stillAvailableMageIds: string[]
}) => {
const baseResult = {
result: [],
seed,
}

if (mage === undefined) {
return baseResult
} else {
const ids = mage.filter((idOrRandom) => typeof idOrRandom === 'string')

const randomTreasureConfigs = mage.filter(
(idOrRandom): idOrRandom is { random: true } =>
typeof idOrRandom !== 'string'
)

return randomTreasureConfigs.reduce(
(acc: RewardResult, _) => {
const filteredMageIds = stillAvailableMageIds.filter(
(mageId) => ids.indexOf(mageId) === -1
)

const mageIdsResult = createIdList(
Expand Down
7 changes: 2 additions & 5 deletions src/components/pages/Expeditions/CreationDialog/ConfigImport/expeditionJsonSchema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// This schema has been created by using https://github.com/YousefED/typescript-json-schema
// To recreate the schema run and replace it in here:
// To recreate the schema run:
//
// typescript-json-schema tsconfig.json ExpeditionConfig --include ./node_modules/aer-types/types/**/*.ts --required --aliasRefs
//
Expand Down Expand Up @@ -86,10 +86,7 @@ export const schema = {
Barracks: {
properties: {
mageIds: {
items: {
type: 'string',
},
type: 'array',
$ref: '#/definitions/Array_1',
Copy link
Owner

@on3iro on3iro Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this wont do.
There are a few more things necessary to make all of this work:

We will probably need to:

  1. Add another type for our initialBarracksConfig so that it is no longer of the type Barracks but the type InitialBarracksConfig in here https://github.com/on3iro/aer-types/blob/master/types/expeditions.ts

The type will probably look like this:

type InitialBarracksConfig = {
   mageIds: [
       { type: 'random' },
       { type: 'custom', id: 'someMage' }
   ],
   supplyIds: string[],
   treasureIds: string[]
}
  1. We then need to create a new version of the aer-types package and publish it to npm (I will need to do that) For now, you could change the type locally on your system, to play around a bit until we are satisfied with the API.
  2. You will need to install the new version with yarn
  3. You might then see new typescript errors in the compiler - so you will probably need to make adjustments to your code
  4. You then need to re-create the json-schema with the command inside the comment above

We need to do this, to make the app as typesafe as possible (during compilation as well as at runtime, when users try to import an expedition)

  1. We also need to change the documentation for initial barracks, so its in line with the change

(7.) It would probably make sense, to also allow treasureIds and supplyIds to be randomly rolled in that scenario. So maybe we should change these in the InitialBarracksConfig as well (which in turn would mean, that you would need to provide additional code for them) - However we can focus on mages now and add that later, if you would prefer to keep this PR streamlined :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need help here, just let me know. We can also make a call over on discord, so I could guide you through necessary steps, if that would help you :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a discord call would be good here to show me some of this process. The other issues above are things I can easily amend before our call but the relationship between the aer-types package and the json-schema are something I'm still pretty fuzzy on.

I'm thinking this weekend is the time that makes the most sense to try to get together over discord. I'm also starting to think we might be in pretty different timezones. I live in Arizona so I use MST (Mountain Standard Time). Not sure what time would maybe work for you or if you want to play it by ear.

And I don't mind trying to get supplyids and treasureids working as well.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool - yeah we definitely would have to check timezones before hand :D

I am currently thinking about migrating aer-types back into this repository (but keep publishing it as a separate type-library to npm...). That would make collaborating way easier.

I probably wont be able to make a call during the upcoming weekend because of the timezone issue (this weekend is rather full either) so maybe the weekend after? I will definitely review your code this weekend and leave a few more comments, though. So that way you should be able to already make a few more adjusments. Would that work for you?

(unfortunately you catched me durng a rather busy time with project dead lines at work as well as lots of family stuff to attend to in my spare time)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(we are nine hours apart btw - so when its 10:00 am in your country its 7:00pm in mine)

Copy link
Owner

@on3iro on3iro Oct 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Quicksav I changed the way this code base works regarding types and data. Both are now part of the repository again (we will still publish them to NPM, so that they can be consumed by other libraries). This will make it way easier to adjust typing while working on a feature, because we no longer need to publish the typings in between etc.

For you to receive these changes, you need to do the following things on your feature branch:

  1. set a git remote for this original repository (in addition to your own fork upstream remote), name it upstream or something like that
  2. than on your feature branch run git pull --rebase upstream dev - that should get you all the changes you need
  3. run yarn again, so that your node_modules are up to date
  4. force push onto your feature branch (rebase alters the git history, hence it's necessary to force push)

Unfortunately I am out of time for now and haven't been able to look at your changes, yet. I will try to do so asap. But if you want you could play around with the typings a bit and try to type the initialBarracksConfig.

If you have any questions, let me know :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having some trouble understanding what I'm supposed to do with this. Unfortunately I think most of my confusion stems from my unfamiliarity with proper git usage. Really just not understanding number 1 of the above steps. Am I supposed to fork the original repository again? Am i just making an 'upstream' branch on my local machine? Or is it that I need to make the 'upstream' branch remote? Sorry for my ineptitude with this I am used to using Visual Studio to handle git commands in a relatively straightforward development environment.

Would having a call next Saturday at ~10AM your time (it'd be Friday after midnight for me) work to go over some of this? I've fixed the other feedback locally but this is the big outstanding thing.

Copy link
Owner

@on3iro on3iro Oct 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, no problem :)

So the thing we are trying to achieve is, to get all the new changes made to this repository (so everything which has been merged to dev) into the feature branch you are currently working on.
To do this, you have to follow a few steps.

The first step outlined above, is that you need this repository as a remote source for fetching changes.
Currently your local repository probably only has a single remote (which is probably called origin). A remote is a server you can connect to and push/pull changes/branches/tags etc. from/to. This single remote targets your fork an github. When you type

git remote -v show into your terminal, you will see something like this:

Screenshot 2020-10-12 at 08 16 54

However the url to will point to your forked repository, not our original aeons-end-randomizer repo.

So what you will have to do, is the following (locally):

  1. run git remote add upstream [email protected]:on3iro/aeons-end-randomizer.git (note, that if you use https instead of ssh your url will look like this: https://github.com/on3iro/aeons-end-randomizer.git) upstream can be any name here, so you could also name it foo or whatever :)
  2. now you have an additional remote
  3. now on your feature branch do the following git pull --rebase upstream dev
  4. this rebases all changes which where made to dev into your feature branch
  5. Now you need to push all of this to your fork, to update this Pull Request. Because rebase changes the git history, you need to do a force push, so on your feature branch do the following: git push -f (note: rebasing is very powerful, because it helps keeping a clean git history, however there are a few gotchas, so if you start rebasing in other personal projects, make sure you know these. If you want to know more, I can teach you :) )

Afterwards the PR should be mergable to dev again, and not longer out of date.
Does that help? :)

Edit: Oh, regarding a pair programming session: unfortunately I can't promise that I will have time on saturday. I might be away during the day. We could either schedule something a few hours later (so it would be saturday morning for you and evening for me or maybe do it on sunday)

},
supplyIds: {
items: {
Expand Down