Conversation
NobodyNada
left a comment
NobodyNada
left a comment
There was a problem hiding this comment.
This looks great, thanks! Just a couple nits.
src/core/mod.rs
Outdated
| input: *const [u8], | ||
| input_ports: *const [InputPort], | ||
| audio_callback: Option<*mut dyn FnMut(&[AudioFrame])>, | ||
| input: &'static [u8], | ||
| input_ports: &'static [InputPort], | ||
| audio_callback: Option<&'static mut (dyn FnMut(&[AudioFrame]) + Send)>, |
There was a problem hiding this comment.
These were raw pointers to make it obvious that they have lifetimes not tracked by the compiler. If you're changing this to use references, please add a comment documenting that these don't really have 'static lifetimes and that we need to be careful when accessing these fields.
(As a matter of style I'd generally prefer to keep them as a raw pointers just as a reminder to be careful when working with these fields, but I'm in the middle of rewriting this whole file for the wasm port anyway, so I don't really care too much what happens to it in the meantime.)
There was a problem hiding this comment.
Removed the references but kept the + Send change on audio_callback.
src/core/mod.rs
Outdated
| pub fn read_memory_byte(address: usize) -> Option<u8> { | ||
| lock_opt()?.get_memory_byte(address).copied() | ||
| } | ||
|
|
||
| pub fn read_memory_le(address: usize, width: u8) -> Option<u64> { | ||
| let mut core = lock_opt()?; | ||
| let mut v = 0; | ||
| for offset in 0..width as usize { | ||
| v |= (core.get_memory_byte(address + offset).copied()? as u64) << (offset * 8); | ||
| } | ||
| Some(v) | ||
| } | ||
|
|
There was a problem hiding this comment.
The core should not expose global state -- the design of mvi is such that all the components are modular, interact via references, and have no global state.
Core internally breaks this model due to the limitations of dynamic libraries. However, the external interface to Core abstracts this away by requiring access via references to hide the global state that exists internally. And once the wasm rewrite is complete, Core will no longer have any global state -- it will be a fully self-contained object that owns an instance of a wasm module. This will make it possible to load multiple Cores in the same process, which is something I'd like to be possible for use cases that embed mvi as a library.
I understand that having global functions simplifies the Rhai callback functions that need to access core state. I think the best way to implement this would be by using NativeCallContext::tag or similar to provide a context object that Rhai callbacks can use to access relevant state.
There was a problem hiding this comment.
Changed it to use the tag feature. Since tags must implement Any they must be 'static, so i used unsafe to extend the lifetime here:
A cleaner option would be to wrap Tas.core into an Arc and a Mutex but it's more boilerplate. I can make the change if you want.
| /// `true` if script must be used instead of address and format | ||
| use_script: bool, | ||
| address: String, | ||
| format: movie::RamWatchFormat, | ||
| rhai_script: String, |
There was a problem hiding this comment.
Can this be factored out into an enum to avoid data redundancy? Maybe this could even just be a RamWatchValue.
There was a problem hiding this comment.
Editor contains both address/format and the rhai script to remember values in text fields and radio buttons when the user switches between the editor modes.
Since `CoreImpl` is `Send`, `audio_callback` must also be `Send`
this runs the library deinitialization
Before, since `reported_error` wasn't set to `None` on close, the popup would reopen every time.
This allows users to enter complex expressions in the RAM watch editor,
using the rhai scripting language
For example, showing samus' speed in hex values:
```rhai
let int = mem::u16le(0xb46).to_hex();
let sub = mem::u16le(0xb48).to_hex();
`${int}.${sub}`
```
hhirtz
force-pushed
the
ram-watch-scripts
branch
from
e3ea1e5 to
53d0b9e
Compare
|
Updated the design of the editor. there is now a radio button to switch between the modes: 
|
2 participants
Following #5
This PR is still a work in progress but the bare functionality is here.
rip rhai highlighting
It also has a bunch of random fixes so it's probably a better read by commit.. i can open other PRs if you want.
TODO
Would also be nice to: