Opened 12 years ago

Closed 12 years ago

Last modified 23 months ago

#8987 closed patch

SCI: Started objectifying the graphics resource manager

Reported by: bluegr Owned by: bluegr
Priority: normal Component: Engine: SCI
Keywords: Cc:
Game:

Description

Hello

This is a fairly big change to SCI, so I wanted to get some opinions first.

Changes: - I've started moving code outside of gfx/resource/res_manager.cpp into gfx/resmgr.cpp. In FreeSCI, it was possible to have several different graphics resource managers, but we only have one, so the code in resource/res_manager.cpp can be merged in gfx/resmgr.cpp (that's the only place it's used in) - I got feedback on this by Lars Skovlund. - It seems that the graphics driver parameter is not needed in gfx_free_pixmap(). That method checks if the screen palette is set, and frees the associated pixmap parameter. I really don't understand why this is necessary, and I only found one place where pixmaps are freed and have invalid palettes: inside gfxr_draw_pic01(), when gfx_free_mode() frees the graphics mode, it frees the view's associated palette too. Setting the view's palette to NULL in this place after freeing it seems to be enough. By removing this parameter from gfx_free_pixmap(), several associated free() methods can be simplified (since the graphics driver parameter is removed from all of them - it was only piggybanked for gfx_free_pixmap(). - I've started objectifying the methods of the graphics resource manager. They all require parts of the graphics resource state, so it made sense to create a class and pass the state to the constructor (plus, all of these methods are parts of the graphics resource manager). I didn't convert all of them yet, because of the previous issue - removing the driver parameter means that more methods can be placed inside the graphics resource manager, which won't be needing the graphics driver parameter.

Are my assumptions correct? Does this kind of objectification make sense?

Comments are welcome

Ticket imported from: #2689887. Ticket imported from: patches/1092.

Attachments (1)

sci.patch (78.8 KB ) - added by bluegr 12 years ago.
SCI objectification WIP (new) - with wjp's suggestion

Download all attachments as: .zip

Change History (6)

comment:1 by wjp, 12 years ago

At first glance it looks like a good direction to take. Is there a reason why you didn't remove the gfx_resstate_t type entirely to absorb it into GfxResManager? (I don't understand your 3rd point, so maybe that answers it.)

Some feedback on specific issues you raised above:

> Setting the view's palette to NULL in this place after freeing it seems to be enough.

That's not right. I can't test it right now, but I suspect the 'view->palette' parameter to gfx_new_mode should be changed to 'view->palette->getref()'.

> That method checks if the screen palette is set, and frees the associated pixmap parameter.

That driver check is indeed unnecessary now. (It may have been necessary with the old palette code.)

by bluegr, 12 years ago

Attachment: sci.patch added

SCI objectification WIP (new) - with wjp's suggestion

comment:2 by bluegr, 12 years ago

Sorry, this sentence: "Setting the view's palette to NULL in this place after freeing it seems to be enough" Should be: "Setting the view's palette to NULL in this place after freeing the graphics mode (which frees the palette reference) seems to be enough"

And you're right wjp, changing "view->palette" to "view->palette->getref()" to gfx_new_mode fixes the issue. Updated the patch with this change

As for removing the gfx_resstate_t... yeah that's the plan, when this is all done :) File Added: sci.patch

comment:3 by bluegr, 12 years ago

I'll upload this then, since my assumptions seem to be correct, and I'll continue working on objectification

comment:4 by bluegr, 12 years ago

Owner: set to bluegr
Status: newclosed

comment:5 by digitall, 23 months ago

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