#8987 closed patch
SCI: Started objectifying the graphics resource manager
| Reported by: | bluegr | Owned by: | bluegr |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCI |
| Version: | 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)
Change History (6)
comment:1 by , 15 years ago
by , 15 years ago
SCI objectification WIP (new) - with wjp's suggestion
comment:2 by , 15 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 , 15 years ago
I'll upload this then, since my assumptions seem to be correct, and I'll continue working on objectification
comment:4 by , 15 years ago
| Owner: | set to |
|---|---|
| Status: | new → closed |
comment:5 by , 6 years ago
| Component: | → Engine: SCI |
|---|
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.)