Opened 12 years ago

Closed 12 years ago

Last modified 11 months ago

#8758 closed patch

ScriptVars class implementation for CinE

Reported by: SF/next_ghost Owned by: sev-
Priority: normal Component: Engine: Cine
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)

scriptvars.patch (7.6 KB ) - added by SF/next_ghost 12 years ago.
ScriptVars against SVN 29859

Download all attachments as: .zip

Change History (10)

comment:1 by fingolfin, 12 years ago

Owner: set to vincenthamm

comment:2 by sev-, 12 years ago

Owner: changed from vincenthamm to sev-

comment:3 by sev-, 12 years ago

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.

comment:4 by bluegr, 12 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 :)

by SF/next_ghost, 12 years ago

Attachment: scriptvars.patch added

ScriptVars against SVN 29859

comment:5 by SF/next_ghost, 12 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 cyxx, 12 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 sev-, 12 years ago

Status: newclosed

comment:8 by sev-, 12 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 digitall, 11 months ago

Component: Engine: Cine
Note: See TracTickets for help on using tickets.