BeeStation / BeeStation-Hornet

99.95% station. 0.05% bees
https://beestation13.com
GNU Affero General Public License v3.0
200 stars 681 forks source link

Some `FAST_REF(thing)` or `REF(thing)` should be replaced to refer its weakref datum, like `FAST_WEAKREF(thing)` #10890

Open EvilDragonfiend opened 7 months ago

EvilDragonfiend commented 7 months ago

Describe the feature request

Some REF(thing) should be replaced to refer its weakref datum - kinda FAST_WEAKREF(thing)

Let's say there is a John /mob[0x0001], and we know its ref value once John /mob[0x0001] is deleted, a memory slot 0x0001 becomes free, be allowed to have a new mob. and then Janne /mob[0x0001] will take the memory slot.

Meanwhile, if something was logged as my_friend = "[0x0001]", it refers to Janne because the mob takes the memory, but real my_friend is supposed to be John.

Thus, there should be a kind of proc that takes a reference value of a type's weakref. created weakref for Jobn will be /datum/weakref [0x900001], and it will be my_friend = "[0x900001]" Even if John is deleted, weakref still exists. Even if weakref refers to [0x0001], its okay. We used a key as John's weakref, and we don't have to resolve it.

If a weakref for Janne is created, it will be /datum/weakref [0x900002], my_friend = "[0x900001]" still refers to John (exactly, John's weakref). thus it solves a bad ref issues.

#define FAST_WEAKREF(thing) FAST_REF(WEAKREF(thing))

FAST_WEAKREF should be that form.

EvilDragonfiend commented 7 months ago

Labeling Bug too since it's potentially buggy.

EvilDragonfiend commented 7 months ago

Example) image

PowerfulBacon commented 7 months ago

Is \ref not unique?

The obvious solution when implementing a game engine is to just increment a counter for refs, or use a UUID. It would take a lot of extra effort with little gain to have a game engine re-use deleted refs.

EvilDragonfiend commented 7 months ago

Is \ref not unique?

Not trully unique because qdeleting releases the memory slot, and new mob will take the slot.

The obvious solution when implementing a game engine is to just increment a counter for refs, or use a UUID. It would take a lot of extra effort with little gain to have a game engine re-use deleted refs.

maaaaybeee? /datum/var/static/counter and /New() { counter++ }??

EvilDragonfiend commented 7 months ago
/datum/proc/unique_ref_count()
    if(!ref_counter)
        ref_counter = ++global_counter
    return ref_counter

/datum
    var/static/global_counter
    var/ref_counter

Maybe this?

PowerfulBacon commented 7 months ago

No, thats not what I mean. I was talking about byond-level implementation details, not something codeable in dreammaker

PowerfulBacon commented 7 months ago

Not trully unique because qdeleting releases the memory slot, and new mob will take the slot.

I guess, this highly depends on the implementation since byond is going to be using its own virtual memory implementation and not physical addressing

EvilDragonfiend commented 7 months ago

Not trully unique because qdeleting releases the memory slot, and new mob will take the slot.

I guess, this highly depends on the implementation since byond is going to be using its own virtual memory implementation and not physical addressing

That's why this issue says about using weakref as a type's true unique ref id, because we don't qdel weakref

image

/proc/main()
  var/datum/D = new()
  D.tag = "first one"
  world.log << "< First datum >"
  world.log << "My tag: [D.tag]"
  var/reftext = ref(D)
  world.log << "My ref: [reftext]"
  del(D)
  D = new()

  world.log << ""
  var/datum/another_datum = locate(reftext)
  world.log << "< Second datum >"
  world.log << "My tag: [another_datum.tag]"
  world.log << "My ref: [ref(another_datum)]"
  world.log << "First datum ref: [reftext]"

As the example I described, It has a pottential issue with it.

PowerfulBacon commented 7 months ago

I see, this may be an issue. Using a weakref wrapper around all objects is a big cost though, if this doesn't cause any issues in practice then it might be worth trying to find issues first.

EvilDragonfiend commented 7 months ago

I see, this may be an issue. Using a weakref wrapper around all objects is a big cost though, if this doesn't cause any issues in practice then it might be worth trying to find issues first.

Not all objects need to do that. For now, a visible first issue is

image

when a mob is deleted and then a new mob is created, some mobs can have them allied even if that mob isn't actually what their ally - as the issue described. and I wonder which cost you're concerning on as which aspect.

If you think it'll be a little bit slow to access, I think it won't be that slow...

#define FAST_WEAKREF(thing) (thing.weak_reference?.cached_ref || FAST_REF(WEAKREF(thing)))

Hopefully?