GothicKit / ZenKit

A re-implementation of file formats used by the early 2000's ZenGin
http://zk.gothickit.dev/
MIT License
47 stars 10 forks source link

transient and opaque instances #79

Closed Try closed 1 year ago

Try commented 1 year ago

This PR introduces 2 new special types for instances:

  1. opaque_instance - almost same as you propose in ticket, intended as replacement for C_SVM, C_FightAi
  2. transient_instance - callback-based workflow, that delegates read/write operations to the engine.

When working on Ikarus support, there are several use-cases:

Try commented 1 year ago

@lmichaelis kindly poking for code-review :)

Try commented 1 year ago

Even though it might introduce a small performance degradation, I think we should instead opt for a more uniform virtual API through which instance members are accessed. Why did you choose to implement it this way?

In principle, it would be nice to have entire memory access into single function, in for example const T* get_member_ptr(...). Unfortunately can't do it for transient_instance as there is no memory backing - just keep to in symbol.

Speaking of virtual functions: can you clarify on how interface can look like?

lmichaelis commented 1 year ago

Speaking of virtual functions: can you clarify on how interface can look like?

Maybe something like:

class base_instance {
protected:
    virtual int* int_storage(size_t offset) = 0;
    virtual float* float_storage(size_t offset) = 0;
    virtual std::string* string_storage(size_t offset) = 0;

    friend class vm;
};

class instance : protected base_instance { /*...*/ };

Which is then implemented as a reinterpret_cast<T*>(reinterpret_cast<uint8_t*>(this) + offset) for normal instances, a reinterpret_cast<T*>(_m_storage.get() + offset) for opaque instances and implemented by an external user for transient instances. get_member_ptr can then have a template specialization for each case (or just inline each case into their respective get_-functions).

You need to differentiate between read an write they you could also use write_int and read_int and so on.

I might be missing something here but since we only access symbol values through symbol::{get_, set_}* this should work just fine.

Try commented 1 year ago

While evaluating virtual-base approach: Put 3x-get and 3x-set functions in base class only for the sake of transient_instance is overkill, I think. get_member_ptr works just fine for normal classes as well as opaque ones.

In interface, parameter size_t offset is not useful for transient_instance. Ideally need to know symbol and index, to implement remapping. Also transient_instance have to bypass binding checks (and maybe array size checks): binding won't be valid, as it's a generic proxy-class.

I've considered to make solution based on virtual [const] void* data() - works for normal/opaque; but impossible to implement for transient.

lmichaelis commented 1 year ago

I've considered to make solution based on virtual [const] void* data() - works for normal/opaque; but impossible to implement for transient.

Okay, I'm happy with that 👍. We're good to merge after making get_{float, int} and set{float, int, string} private or protected.

Try commented 1 year ago

Okay, I'm happy with that 👍. We're good to merge after making get_{float, int} and set{float, int, string} private or protected.

This is done; also decide to change transient_instance a bit and make it more consistent with everything else.