Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

addProfile() #126

Closed
noonereally opened this issue Jun 1, 2015 · 14 comments
Closed

addProfile() #126

noonereally opened this issue Jun 1, 2015 · 14 comments

Comments

@noonereally
Copy link
Collaborator

To continue noonereally#19
And #122

I think now that the best option is to use '+' from @WloBeb suggestion or a number as a last (or maybe first will be better) member of a level array to denote if it should be replicated, when '+' will replicate until met with a defined level.
And a number can denote until what level to replicate, for example:

levels : {
      5: [ "something", "+"],
      10: ["Something else", 16],
      20: ["more other somethings"],
      21: ["more other somethings"],
      22: ["more other somethings"],
      23: ["more other somethings"],
}

Will result in:

levels : {
      5: [ "something"],
      6: [ "something"],
      7: [ "something"],
      8: [ "something"],
      9: [ "something"],
      10: ["Something else"],
      11: ["Something else"],
      12: ["Something else"],
      13: ["Something else"],
      14: ["Something else"],
      15: ["Something else"],
      16: ["Something else"],
      20: ["more other somethings"],
      21: ["more other somethings"],
      22: ["more other somethings"],
      23: ["more other somethings"],
}

Then extending it over default will end like:

levels : {
      1: [ "default tasks"],
      2: [ "default tasks"],
      3: [ "default tasks"],
      4: [ "default tasks"],
      5: [ "something"],
      6: [ "something"],
      7: [ "something"],
      8: [ "something"],
      9: [ "something"],
      10: ["Something else"],
      11: ["Something else"],
      12: ["Something else"],
      13: ["Something else"],
      14: ["Something else"],
      15: ["Something else"],
      16: ["Something else"],
      17: [ "default tasks"],
      18: [ "default tasks"],
      19: [ "default tasks"],
      20: ["more other somethings"],
      21: ["more other somethings"],
      22: ["more other somethings"],
      23: ["more other somethings"],
      24: [ "default tasks"],
      25: [ "default tasks"],
}
@dlebedynskyi
Copy link
Contributor

might be better idea for composition as following

levels : {
      5: ['something'],
      6: '+10',
     15: '+17',
     20 : [ 'something']
}

will resulting in 
levels : {
   1: ['default'],
   2: ['default'],
   3: ['default'],
   4: ['default'],
//  +10,
   5 : ['something'],
   6 : ['something'], 
   7 : ['something'], 
   8 : ['something'], 
   9 : ['something'], 
 10 : ['something'],
// default
 11 : ['default again 11'],
 12 : ['default again 12'],
 13: ['default again 13'],
 14: ['default again 14'],
// 15: + 17
 15: ['default 15'],
 16: ['default 15'],
 17: ['default 15'],
// back to defaults
 18: ['default 18'],
 19: ['default 19'],
//special no +
 20 : [ 'something'],
//default
 21 : [ 'default'],
 22 : [ 'default'],
 23: [ 'default'],
 24 : [ 'default'],
 25 : [ 'default'],

}

Tell me what you think. Assign to me pls after

@noonereally
Copy link
Collaborator Author

Few questions:

  1. What are the rules. looks like at 6 we duplicate the line before it, but at 15 we duplicate the same line and not the one before.
  2. at some point I thought to use the +n to have it duplicate n lines from now so +3 will duplicate the current 3 times. So the + somewhat confusing now for me, but generally if you leave only numbers I don't think we should leave the +.

I like the idea of using the empty line. So perhaps we can combine all of it and have:

levels : {
      5: ['something 5'],
      6: '+2',
      10: ['something 10'],
      11: '+',
      15: ['something 15'],
      16: 19,
      20 : [ 'something']
}

And result will be (before extend default):

levels : {
      5: ['something 5'],
      6: ['something 5'],
      7: ['something 5'],
      10: ['something 10'],
      11: ['something 10'],
      12: ['something 10'],
      13: ['something 10'],
      14: ['something 10'],
      15: ['something 15'],
      16: ['something 15'],
      17: ['something 15'],
      18: ['something 15'],
      19: ['something 15'],
      20 : [ 'something']
}

So we have '+' extend until there is defined level (with tasks),
'+2' will add 2 lines or until hits defined level,
and a number will add until that level or a defined level.

The line that will be replicated will always the one before the sign ('+' , '+n' , n)
And the line with the sign will be counted in for the +n case.

If there are other suggestions people (those that around), please post.
@WloBeb @BigRedBrent @Phr33d0m

@Phr33d0m
Copy link
Owner

Phr33d0m commented Jun 2, 2015

I'm still unsure why we need any of the ('+' , '+n' , n) stuff. Isn't this just simpler to write?

levels : {
      5: ['something 5'],
      8: null,
      9: null,
      10: ['something 10'],
      15: ['something 15'],
      19: null,
      20: ['something 20']
}

Will be expanded to:

levels : {
      1: ['default 1'],
      2: ['default 2'],
      3: ['default 3'],
      4: ['default 4'],
      5: ['something 5'],
      6: ['something 5'],
      7: ['something 5'],
      8: ['default 8'],
      9: ['default 9'],
      10: ['something 10'],
      11: ['something 10'],
      12: ['something 10'],
      13: ['something 10'],
      14: ['something 10'],
      15: ['something 15'],
      16: ['something 15'],
      17: ['something 15'],
      18: ['something 15'],
      19: ['default 19'],
      20: ['something 20']
}

In any case, what if I just want to prepend a task for 15-20 professions? Can we have something like:

prepend: true,
levels : {
      15: ['something 15'],
      19: null,
      20: ['something 20']
}

Becomes:

prepend: true,
levels : {
      // ... tasks from default 1-14
      15: [/* default 15 tasks added here */, 'something 15'],
      16: [/* default 16 tasks added here */, 'something 15'],
      17: [/* default 17 tasks added here */, 'something 15'],
      18: [/* default 18 tasks added here */, 'something 15'],
      19: ['default 19'],
      20: [/* default 20 tasks added here */, 'something 20']
}

@noonereally
Copy link
Collaborator Author

This can also work, it depends what we choose as default behavior, in the first examples the default is not to duplicate lines, so you need a special marker to duplicate.
In this it will duplicate by default, so we need stop markers.

About prepend, I'm inclined against profile flags, this are more like preprocessing directives and affect how addProfile works, IMO they shouldn't stay in the profile.

If we default to duplicate the lines we can use the + at the end of the level to prepend. It will be clear what it does and removed in the process.

so we get 3 'passes', duplicate, prepend, extend.

@Phr33d0m
Copy link
Owner

Phr33d0m commented Jun 2, 2015

@noonereally I like that idea more than mine. Adding , '*' or , '+' at the end to append (or at the beginning to prepend) sounds like a nice idea.

For simplicity's sake - I'm inclided to vote on duplicating lines if necessary.

And a way to minimize memory footprint you may ask? Load in memory only professions (and their profiles) that are in use. For example if I have all my characters doing only Leadership - load only that profession with it's profiles. If you happen to decide on this you can avoid duplicating lines as you can fetch lines not-defined-in-that-profile from the parent's profile.

I'm not really sure if that was clear enough so if it wasn't ask me to clearify.

@noonereally
Copy link
Collaborator Author

Ok, we can use the + just to insert the base profile tasks into that place in the array, put it at the beginning and it's prepend, at the end and it's append, or anyware and it will put it there. Easy to implement to.
indexOf, two splices and one join.

"For simplicity's sake - I'm inclided to vote on duplicating lines if necessary. "
This means we duplicate lines by default ?
Until met with defined line (null in case of early stop).

About memory, I don't think it's an issue, to interpret the script it's loaded fully anyway, and it's 290k in whole with spaces and comments. It can even fit in 640k ;)
Just for comparison the background decal image is ~105kb, the background itself constructed of several images about ~50k, 70k, and 100k
Each tiny icon used for items is around 10k.
The promo div background with the armored bulette is 210k.

@dlebedynskyi
Copy link
Contributor

  1. What are the rules. looks like at 6 we duplicate the line before it, but at 15 we duplicate the same line and not the one before.

this one is wrong in my explanation should be line before as in 6. Agree with your suggestion. This is what i was thinking about as well.

Easy to implement to.indexOf, two splices and one join.

@noonereally do you want to implement it yourself? Just make sure not to mix

 6 : '+ 3',  
vs  
 6 : ['...', '+', 'task_extra']
vs 
6 : ['task_extra','+', '...']

@noonereally
Copy link
Collaborator Author

do you want to implement it yourself? Just make sure not to mix.

Was just 'thinking aloud', I'll leave the implementation to you if you are willing.

I think the append with the '+' in the level is closed.

6 : ['task_extra1', '+', 'task_extra2']
7 : ['+', 'task_extra2']

Will become

6 : ['task_extra1', 'tasks from base',.., 'task_extra2']
7 : ['tasks from base',.., 'task_extra2']

(And the '+' will be removed if there is nothing to insert or no base)

Remains to decide if we want to have default duplication of lines:
and we use nulls to stop the duplication (or until it hits another defined line)

levels : {
      5: ['something 5'],
      7: null,
      10: ['something 10'],
      15: ['something 15'],
      20: ['something 20']
}

or use markers to duplicate, like the ('+' , '+n' , n)

I think that for now we have only the gathering profiles that are duplicated, in most cases the base extend/override is the more important part and we don't duplicate.
But also, the override blocks usually come in a group like

      20: ['something 20']
      21: ['something 21']
      22 ['something 22']
      23: ['something 23']
      24: ['something 24']
      25: ['something 25']

So default duplication shouldn't affect it.

@noonereally
Copy link
Collaborator Author

After looking again at the profiles, we have a lot of gathering and alchemy that can benefit from replication and all the base override is at the high level and come in blocks.
So I'm inclined towards duplicated lines by default. My main concern is that using lines with null will be not clear enough as replication stop.

@dlebedynskyi
Copy link
Contributor

Here is a plunker to play http://plnkr.co/edit/DwXElakY47SdvIhusxF0?p=preview
Please test guys so that it would satisfy everyone @noonereally @Phr33d0m @BigRedBrent @WloBeb

One thing to note and document

For 1st level for situation when

  1. new profile does not have full list. No base profile
  2. new profile has "+" as lvl 1 level.

s1 . for 1 I m gonna just put empty list
s2. for 2 If base profile defined use that one. otherwise fallback to s1.

for lvl > 1

  1. if new profile is empty array due to and base profile is not defined gonna copy one level before -> duplicate lvl -1.

dlebedynskyi added a commit to dlebedynskyi/NW-Profession-Bot that referenced this issue Jun 8, 2015
@noonereally
Copy link
Collaborator Author

Looks good to me.

Just small question (it's from the old code)

    var professionBase = {
        taskListName: profession, // Friendly name used at the UI
        taskName: profession, // String used at the gateway
        taskDefaultPriority: 2, // Priority to allocate free task slots: 0 - High, 1 - Medium, 2 - Low
        taskActive: true,
        taskDefaultSlotNum: 0,
        taskDescription: "",
        profiles: []
    };

if profession is the object, wouldn't it create some sort of cyclic reference ?
taskListName: profession

@dlebedynskyi
Copy link
Contributor

if profession is the object, wouldn't it create some sort of cyclic reference ?
taskListName: profession

should be string. I don't think so. Will check actually

@noonereally
Copy link
Collaborator Author

I thought it's a string only, but

    var professionSet = (typeof profession === 'object')
        ? jQuery.extend(true, professionBase, profession)
        : definedTask[profession] || professionBase;

so I thought to ask.

@dlebedynskyi
Copy link
Contributor

when into release. @Phr33d0m close please as done

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants