#8758 closed patch
ScriptVars class implementation for CinE
| Reported by: | SF/next_ghost | Owned by: | sev- |
|---|---|---|---|
| Priority: | normal | Component: | Engine: Cine |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
Simple ScriptVars implementation merged with current code. There're also two bugs fixed (memory overrun in globalVars causing memory corruption of overlayHead and an older "fix" resulting in memory leak).
Ticket imported from: #1848173. Ticket imported from: patches/863.
Attachments (1)
Change History (10)
comment:1 by , 16 years ago
| Owner: | set to |
|---|
comment:2 by , 16 years ago
| Owner: | changed from to |
|---|
comment:3 by , 16 years ago
comment:4 by , 16 years ago
This patch consists of 3 parts: - ScriptVars implementation (NOT added yet to the repository) - Change from the C style malloc/free to the C++ style new/delete (added to the repository) - commit #29841 - Proper fix for bug #1733238 - "FW: crash in copier room" (added to the repository) - commit #29852
Overall, very nice work indeed, but please fix your patch as sev suggested so that your ScriptVars implementation can be added to the repository as well :)
comment:5 by , 16 years ago
Here's the improved patch. thebluegr, next time please make sure that each new is paired with delete and not with free(). You've missed 3 free()s of overlayHeadElement in object.cpp. I know it doesn't matter yet but it might very soon. Fixed in the patch.
I've also removed set() and get() because they're not used and I don't plan to use them anymore. The default ScriptVars constructor has been marked as explicit to avoid strange bugs if someone wrote something like vars = 0 instead of vars[i] = 0. File Added: scriptvars.patch
comment:6 by , 16 years ago
> Take a look at opcode functions in script.cpp. ::get*Var() and > ::set*Var() would require rewriting many of them, sometimes with awkward > results. ScriptVars class can be used for localVars, globalVars and
You're right on this, if you don't want to rearrange the code and keep it like it is today ; it's probably better to do it that way
> maybe even for script stack without changing a single line of opcode
there's no "script stack" in cinematique. Just a table of offsets indexed by the label number (this should be renamed...)
> functions. On top of that, you get almost free sanity checks on any *Var > access. That's how I've found the real cause of bug #1733238.
You would have the same free sanity checks with extra functions.
Anyway, my voice has little to no interest, I am not the engine maintainer.
comment:7 by , 16 years ago
| Status: | new → closed |
|---|
comment:8 by , 16 years ago
Thanks, Martin. The patch now is in trunk. It rot a bit, so I had to perform manual merge. Please, continue your work.
comment:9 by , 5 years ago
| Component: | → Engine: Cine |
|---|
Please, check http://wiki.scummvm.org/index.php/Code_Formatting_Conventions document.
Particularly we do not put spaces in braces, i.e. fHandle.writeUint16BE(vars[i]); not fHandle.writeUint16BE( vars[i] );.
Also we put spaces after built in keywords:
for (unsigned int i = 0; i < len; i++) {, not for( unsigned int i = 0; i < len; i++ ) {
Note that class variables should start with underscore '_', i.e. size and vars of ScriptVars has to be _size and _vars.
Also we have defined type uint for unsigned int.
Other than that the patch looks really nice and I think that it could even go before release as it is quite straightforward.