RanvierMUD / core

Core engine code for Ranvier
https://ranviermud.com
MIT License
45 stars 40 forks source link

Other issues with randomly generated items #31

Closed ratacat closed 5 years ago

ratacat commented 5 years ago

Okay, I'm deleting original comment here to provide a more clear idea of things.

The item hydration path overview looks like this Inventory.hydrate -> state.ItemFactory.create -> EntityFactory.createByType -> Item.constructor

In Inventory.hydrate, the call to the ItemFactory.create is only passing in the entityReference to be used in the initial call of creating the item, even though Inventory.hydrate has the serialized item Definition. Thus none of the later methods are even accessing the serialized data. I took a shot at addressing this by adding in the serialized definition to this chain...

  hydrate(state, belongsTo) {
    // Item is imported here to prevent circular dependency with Item having an Inventory
    const Item = require('./Item');

    for (const [uuid, def] of this) {
      if (def instanceof Item) {
        def.belongsTo = belongsTo;
        continue;
      }

      if (!def.entityReference) {
        continue;
      }

      const area = state.AreaManager.getAreaByReference(def.entityReference);
      //==============added def as third argument on line below===============
      let newItem = state.ItemFactory.create(area, def.entityReference, def); 
      newItem.uuid = uuid;
      newItem.belongsTo = belongsTo;
      newItem.initializeInventory(def.inventory);
      newItem.hydrate(state, def);
      this.set(uuid, newItem);
      state.ItemManager.add(newItem);
    }
  }
}

then in ItemFactory

create(area, entityRef, def = {}) { // <-- added def here===========
    const item = this.createByType(area, entityRef, Item, def); <-- and here============
    item.area = area;
    return item;
  }

and EntityFactory

createByType(area, entityRef, Type, def = {}) { // <--- here=============
    const definition = this.getDefinition(entityRef) || def; <--- here===========
    /*
    if (!definition) {
      throw new Error('No Entity definition found for ' + entityRef)
    }
    */
    const entity = new Type(area, definition);

    if (this.scripts.has(entityRef)) {
      this.scripts.get(entityRef).attach(entity);
    }

    return entity;
  }

Which seems to successfully rebuild the items from the serialized data, instead of from the entityReference.

The only issue remaining after this that I've found with making randomly generated equipment survive serialization, is that it currently seems to lose it's TYPE, which is a SYMBOL. I attempted to add type: this.type, to the Item.serialize method...however it would appear that JSON.stringify will strip out any properties with symbols for a value. So Item.constructor method will set the TYPE of the item to Object as it's default since the serialized item no longer has a type. I haven't figured out what to do about this yet.

ratacat commented 5 years ago

Also attaching my prototype for random item generation if you want to test against it https://www.dropbox.com/s/asz4qsefqjmfrtn/enceladus-random-generators.zip?dl=0

You might have to change the broadcast requires, as I am using custom broadcast.

shawncplus commented 5 years ago

A few things:

none of the later methods are even accessing the serialized data.

I don't know what you mean here and haven't heard a convincing argument for why you would need it.

The only issue remaining after this that I've found with making randomly generated equipment survive serialization, is that it currently seems to lose it's TYPE, which is a SYMBOL.

I feel like if you're using item as the base for your randomly generated item and you're changing it so much during generation that you're changing the type then you should use a different base item.

ratacat commented 5 years ago

Have you tried this with any item that doesn't have a valid entityReference in YAML/datasource? Because I think you'd see what I mean.

I feel like if you're using item as the base for your randomly generated item and you're changing it so much during generation that you're changing the type then you should use a different base item.

Currently, even though you added name, type, keywords, roomDesc to Item.serialize. None of those are being hit, they're being bypassed entirely. it's still using the entityReference lookup in yaml to populate those values. I don't think that is what you intend.

If you walk through creating an item with no valid entityReference data in YAML, and watch the code try to hydrate it on login, you'll see it.

None of those serialized properties(name, keywords, type, roomdesc) are being using, it's using the values returned from the item definition yaml by way of the entityReference lookup. Additionally, after you see that, if you modify things so that the hydration chain DOES use the serialized inventory data instead of the entityReference/Item Definition data, then you'll find that JSON.stringify(in the JSON datasource file) eats all properties with symbols, see https://stackoverflow.com/questions/45152582/javascript-object-with-symbol-property-gets-removed-by-stringify

I think it's not quite working as you intend, that is all I am trying to say. Not saying that I need anything different (because I can just change my copy of core and manage the differences, I do this already)

shawncplus commented 5 years ago

At the moment it's very much working as intended. The current engine does not support the dynamic creation of items without a base item which has a real entityReference.

The reason is exactly the thing you've ran into here. If you try to create an item out of thin air with an invalid, or non-existent, base item when that gets serialized to a player you've essentially created a null pointer because it doesn't have a valid entityReference. Which means that when the player logs in that item will vanish into thin air if not crash the server entirely.

The way the hydration system was originally made was intended for a type of random generation wherein you would have a real base item and the randomly generated parts would modify properties while keeping the base item the same. Think WoW/Diablo style affixes "Iron Sword of the Fox". Whole-cloth generation is not a workflow I plan on supporting at the moment. If you want to randomly generate items do so from a real base item from a datasource with an entityReference.

If desc/keywords/roomDesc/etc. aren't being hydrated properly that's a separate thing and could be a bug. This section should be doing what you describe and seemed to work when I tested it https://github.com/RanvierMUD/core/blob/master/src/Item.js#L215 if not let me know

ratacat commented 5 years ago

The reason is exactly the thing you've ran into here. If you try to create an item out of thin air with an invalid, or non-existent, base item when that gets serialized to a player you've essentially created a null pointer because it doesn't have a valid entityReference. Which means that when the player logs in that item will vanish into thin air if not crash the server entirely.

Cool, that's fine! Thanks for the help Shawn, I'm not trying to be a pain in the ass!
I'll use a base item definition in the zone. That's a great solution for me. I'm closing this issue and have opened #34 as a hopefully a more specific example of the bug I was trying to talk about.