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.

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

Add docs, playable demo and a few fix/changes to the engine #88

Merged
merged 85 commits into from
Sep 4, 2016

Conversation

Omikhleia
Copy link
Collaborator

@Omikhleia Omikhleia commented Aug 28, 2016

Greetings,

I am suggesting my master branch tag 3.1-RC for a pull request.

Note that the version in package.json is still 3.0.0, I leave it upon you to decide whether it must be upgraded.

Reviews and/or comments are obviously accepted :)

Details of the changes follow hereafter.

Code has been checked with eslint, and via a few playtest sessions.

Changes overview

  • Partial change write manuals #52 : New set of documentation (though it could of course still be expanded). Note: I have the docs in a sub-directory (rather than on a wiki), as I personally find it more convenient to update whenever the code changes.
  • Fix Destroying an inherited object causes a bit of havoc #84
  • Change Command parser extension suggestions #85 (partial) : Support for singularizers ('the', 'any', 'a', 'an') and selection by ordinal number ('1.item', 'item.1', 'item 2'). Note: Globalizers (i.e. 'all') are not supported yet (basic support is here, but eventually resulting in 'ambiguous' as before for multiple match), as this would likely require changes in the verb execution logic, but also changes in verb qualifiers, and therefore changes to the client. I refrained from making such changes, since the current system as proposed is minimally ok for disambiguating objects with identical matches, and improves playability without requiring client editor changes.
  • Fix Some strange crash with chokidar watcher #86
  • Change Command failure customization suggestion #87 (partial) : Add onPlayerCommandFailed() system hook for in-game customization. Note: I am still unsure the rest of the ideas in Command failure customization suggestion #87 are really pertinent, so I opted for the minimalist approach.
  • Change : Allow on-disk file storage to be organized in sub-directories, with '' as pseudo-logical separator in object identifiers. Note: tested on Linux, but without long / exhaustive testplay sessions. Mostly tested on Windows, henceforth. Not tests on MacOS or other systems. The feature is by no mean mandatory (i.e. not using '' in identifiers results in a flat on-disk structure as before) - except for player objects, which are placed in the 'players_' hierarchy and hence in a 'players/' sub-directory). That's actually the only place (beside FS DB / Moo DB) where the engine uses this features. All other 'core' objects (such as 'system', 'ambiguous', etc.) have been left at the top-level).
  • Besides the above-mentioned changes or fixes, it also comes with a small demonstration world and library, so that one may start experimenting with something.

Change details

README.md

doc/...

  • Programming guide
  • Customization guide
  • Demonstration memorandum

demo/...

  • A whole new demonstration world, illustrating as many features as possible, notably including expanded in-game help, command delegation, basic NPC (sellers and a moving mouse), verbMissing seller rooms, extraMatchObjects, etc.

package.json

src/controllers/player-controller.js

src/controllers/user-controller.js

  • Ensure player objects to be created in "players/" as per change "Allow on-disk file storage to be organized in sub-directories".

src/lib/context.js

src/lib/fs-db.js

src/lib/idify.js

  • Allow '_' in identifiers, as per change "Allow on-disk file storage to be organized in sub-directories".

src/lib/moo-db.js

  • Change "Allow on-disk file storage to be organized in sub-directories". That's where almost of this change is, since that's the place where almost all path-related code was.

src/lib/parse.js

src/lib/world-object-class-builder.js

*src/lib/world-object-proxy-builder.js *

@@ -1,11 +1,14 @@
const vm = require('vm');

const parse = require('./parse');
const parse = require('./parse').parseSentence;
const noun = require('./parse').parseNoun;

Choose a reason for hiding this comment

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

Unable to resolve path to module './parse' import/no-unresolved

@Omikhleia
Copy link
Collaborator Author

Omikhleia commented Aug 29, 2016

Fixed (and slightly expanded) the travis/coverage tests, now all passed (tag 3.1-RC2)

I am unsure how to handle the hound reports - they all are on the demo, which should either be excluded from them, or have different rules (everything is normal there, as far as I can tell) - but that's new to me, I have no clue how to do this.

EDIT: Just added demo to .eslintignore, doh.
Still a few "unable to resolve path" issues, I am at loss here, since it loads ok when running.

@@ -1,4 +1,6 @@
const C3 = require('./c3');
const idify = require('./idify');
const parseNoun = require('./parse').parseNoun;

Choose a reason for hiding this comment

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

Unable to resolve path to module './parse' import/no-unresolved

@vitogit
Copy link

vitogit commented Aug 30, 2016

Great work @Omikhleia , I was trying to do something similar to this repo, but you and @doughsay did a much better job.
I´m trying to make the demo work but seems like something is missing. Could you help me? I'm on a mac osx using chrome.

I think is related to the loading moodb subfolders or something. This is my .env

PORT=8889
LOG_LEVEL=trace
MAINTENANCE_MODE=false
USER_DB_FILE=demo/users.json
WORLD_DIRECTORY=demo/world

There are some warnings when loading things, like

[2016-08-30T19:15:15.606Z]  WARN: rjs-server/7643 on TopTiers-Mini-3: tried loading object from fsdb, but failed (directory=demo/world, objectId=areas)
    SyntaxError: Unexpected token u in JSON at position 0
        at Object.parse (native)
        at MooDB.loadObject (/Users/TopTierlabs/apps/mush/room.js/src/lib/moo-db.js:70:27)
        at ids.forEach.id (/Users/TopTierlabs/apps/mush/room.js/src/lib/moo-db.js:42:12)
        at Array.forEach (native)
        at MooDB.load (/Users/TopTierlabs/apps/mush/room.js/src/lib/moo-db.js:41:9)
        at new MooDB (/Users/TopTierlabs/apps/mush/room.js/src/lib/moo-db.js:21:10)
        at RoomJSServer.setupState (/Users/TopTierlabs/apps/mush/room.js/src/lib/room-js-server.js:22:15)
        at new RoomJSServer (/Users/TopTierlabs/apps/mush/room.js/src/lib/room-js-server.js:16:10)
        at Object.<anonymous> (/Users/TopTierlabs/apps/mush/room.js/src/index.js:9:16)
        at Module._compile (module.js:556:32)

In the client I pointed to the localhost server, I created the user and player without problems, and entered in play mode, but when I write 'say hello' it's return 'I didn't understand that': and the error is

[2016-08-30T19:20:36.210Z]  WARN: rjs-server/7643 on TopTiers-Mini-3: error running hook (socketId=/#sh2wHOlxCnPRW8gqAAAA, user=a, player=players_a)
    TypeError: player.tell is not a function
        at Proxy.onPlayerCommandFailed (evalmachine.<anonymous>:21:10)
        at World.runScript (/Users/TopTierlabs/apps/mush/room.js/src/lib/world.js:86:59)
        at Proxy.fn (/Users/TopTierlabs/apps/mush/room.js/src/lib/deserializer.js:33:22)
        at Verb::system.onPlayerCommandFailed:1:32
        at ContextifyScript.Script.runInContext (vm.js:35:29)
        at Object.exports.runInContext (vm.js:67:17)
        at World.run (/Users/TopTierlabs/apps/mush/room.js/src/lib/world.js:82:15)
        at PlayerController.onRunVerb (/Users/TopTierlabs/apps/mush/room.js/src/controllers/player-controller.js:114:16)
        at PlayerController.runCommand (/Users/TopTierlabs/apps/mush/room.js/src/controllers/player-controller.js:73:16)
        at PlayerController.onInput (/Users/TopTierlabs/apps/mush/room.js/src/controllers/player-controller.js:28:12)

Also the tab key don´t work for mode cycling.

@Omikhleia
Copy link
Collaborator Author

@vitogit
Hmm my bad then, I might indeed have trimmed two many / in path names or broken some regexp in the Moo DB: the "but failed (directory=demo/world, objectId=areas)" message in your logs is suspicious, that the object id name was apparently truncated in a wrong way, it should have been something such as 'areas_somearea_someplace'). I have mostly tested on Windows, which gave me bad time with backslashes in path names, and might have done a late change that broke path handling in Unixes...
Thanks for testing and reporting, I'll have access to my Linux box next week-end and will check this... The pull request was probably a bit early thus, apologies to @doughsay...

@vitogit
Copy link

vitogit commented Aug 31, 2016

@Omikhleia I tested on windows and found some problems
1)I had to delete the npm-shrinkwrap.json file because it was trying to install chokidar 1.4.3 instead of 1.6. The old version cause problems on windows with fsevents.
2)npm start don´t work because the script uses unix syntax for the folder structure, probably using join.path could be better for cross-platform

3)I think I found the problem with the object loading.
if you check the db size in FsDb after loading, it will say 13 on Linux/mac, but around 78 in Windows. But on linux/mac the tree structure is correct, you can check it with inspectTree. So it´s seems that on windows (for some reason) is listing every item in the db in the same branch(root), and not creating the tree structure.
So the solution is to check why on windows is happening that, and fix to use the tree structure, and then change the load method on mooDb to load the tree structure recursively. I will try to fix it, but don´t have enough time.

@vitogit
Copy link

vitogit commented Aug 31, 2016

@Omikhleia Changing the load method in moo-db to

  load(dir = '') {
    const ids = this.fsdb.lsDirs(dir);
    ids.forEach(id => {
      const filepath = path.join(dir, id); 
      this.load(filepath)
      this.loadObject(filepath.replace(/[\/\\]/g, '_'));
    });
  }

fix the problem in my mac, and now is working fine. All objects are loading, there are some warning because is trying to load some folders that don´t have json, like demo/world/players, but that doesn't affect gameplay.

@Omikhleia
Copy link
Collaborator Author

Omikhleia commented Sep 1, 2016

Thanks for the testing and feedback, @vitogit. Regarding your points:

  1. npm shrinkwrap willl have to be regenerated and checked on a clean install. Point noted.
  2. Yes, I didn't focus on that (i.e. currently starting with a separate .bat which i've not committed). There are other items in package.json to address if we want to make it fully Win-compliant (e.g. the dev/null in the tests sections) eventually, but that may possibly wait.
  3. The reason why Win and Unixes behave differently here is that mapFor() / mapForDir() in FS DB split paths at '/', which doesn't occur on Win (I overlooked this, at some point). But on the other hand, fixing that (along with your change above) requires MOO DB to indeed work recursively, which I don't find really satisfying - What MOO DB should actually need to start-up is only the list of ids for objects to load (which may remain "flat" in a way), not the whole tree (of possibly empty dir levels). I'll have an attempts at refactoring the code in that direction.

EDIT: One path for refactoring is to work with POSIX filenames only, to avoid all those / vs. \ issues. I always found that the node.js "path" module was bad by design ("The default operation of the path module varies based on the operating system on which a Node.js application is running." - which makes the default API inconsistent...).

@Omikhleia
Copy link
Collaborator Author

Now running on both Linux and Windows without the changes made for one breaking the other, and all path names behaving identically ^^. Everything seems to be running ok.

Finally kept the recursive approach from @vitogit above.

@@ -84,7 +97,7 @@ class FsDb {

mapFor(filepath, create = true) {
const getCallback = create ? this.createOrGetMap.bind(this) : this.getMap.bind(this);
const keys = this.toRelpath(path.dirname(filepath)).split('/').filter(x => x);
const keys = this.toRelpath(path.posix.dirname(filepath)).split('/').filter(x => x);
Copy link

Choose a reason for hiding this comment

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

Instead of using path.posix so many times, I think you could change the requiere to requiere: const path = require('path').posix; and just use path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Point taken.

@doughsay
Copy link
Owner

doughsay commented Sep 3, 2016

Hey, thanks for the contributions! I've cleared out the newly ignored hound comments for the demo world. I'll investigate why the 5 remaining get complaints from Hound but not when run locally. If I can't figure it out, I'm just going to disable Hound. Seems more trouble than it's worth...

@doughsay
Copy link
Owner

doughsay commented Sep 3, 2016

Let's just try disabling the no-resolved rule for eslint to get hound to stop bothering us about it.

I think I will eventually just turn off hound, since I have no control over what version of eslint it's running under the hood. Better to have full control over it and run it manually from within travis the same way we'd run it locally.

@doughsay
Copy link
Owner

doughsay commented Sep 3, 2016

Ok, well I've just disabled hound altogether cause I realized I was already running eslint inside travis. So that will catch any linting errors; no need to also include hound.

@doughsay
Copy link
Owner

doughsay commented Sep 3, 2016

Rebase on my latest master branch to get the removal of hound.yml and re-rerun the checks.

- builder wand
- food and drinks
- closeable/lockable object
- Slightly different hierarchical model
…n subfolders

Seems to work fine with the demo mudlib, but probably some more work do do... Too many conversions between ids and paths between MooDB and FsDb
[DOC] Fix tables in markdown for GitHub...
Markdow tables and linebreaks, etc. - behaving differently than e.g. http://dillinger.io/
Shh, viewing on GitHub seems to break with things such as Array.<WorldObject>... Removing the brackets, then.
Do I hate markdown?
... and change name of inn as a tribute to JRRT.
From module "chalk": use bright blue on Windows as the normal blue color is illegible  i.e. escape 94m instead of 34m.
… print-test regarding colored string:

From module "chalk": use bright blue on Windows as the normal blue color is illegible  i.e. escape 94m instead of 34m.
…tween Win32 and Unix

Fix Moo DB to recurse into the FS DB tree
@doughsay
Copy link
Owner

doughsay commented Sep 4, 2016

Thanks for the contributions! I'm going to go ahead and merge this. There are a few places I'd still like to clean things up, but that can come later.

@doughsay doughsay merged commit c7f3f79 into doughsay:master Sep 4, 2016
@doughsay doughsay mentioned this pull request Sep 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some strange crash with chokidar watcher Destroying an inherited object causes a bit of havoc
4 participants