Author Topic: Let's talk about bmpman  (Read 14125 times)

0 Members and 1 Guest are viewing this topic.

Offline The E

  • He's Ebeneezer Goode
  • Moderator
  • 213
  • Nothing personal, just tech support.
    • Steam
    • Twitter
Let's talk about bmpman
Does anyone actually like that thing? I know I don't, and it seems we're starting to run into its limitations. So let's talk about making it better.

Personally, I think these are the goals we should aim for:

Interface compatibility - By which I mean the interface as defined by bmpman.h, and its implicit guarantees (Like the guarantee that if the bitmap at ID x is an animation, IDs x+1 through x+n can be used to access individual frames). We should not break this any more than necessary.
No more of this "bmpman slots" malarkey - we should not have to iterate over some massive structure to figure out where a given bitmap can fit in
Handle animations as single items - as opposed to decomposing them into frames and jamming them into some structure
BMPMAN IDs should be consistent for the lifetime of the program - As far as the outside of bmpman is concerned, bitmaps should always be accessible using the IDs they were given the first time they were loaded.

Floor is open for discussion and suggestions.
« Last Edit: April 03, 2015, 05:30:42 am by The E »
If I'm just aching this can't go on
I came from chasing dreams to feel alone
There must be changes, miss to feel strong
I really need lifе to touch me
--Evergrey, Where August Mourns

 

Offline m!m

  • 211
Re: Let's talk about bmpman
After a discussion on #scp here is what we came up with:
  • Use an int for the ID and increment it every time we need a new slot (animations are created in one call so for a frame i the ID is startID + i)
  • Use a std::/boost::unordered_map to map from an ID to the underlying data structure. This means that we don't have an upper limit of how many entries there can be. Accessing any entry could be done in constant time.
  • If animations should be handled as single items then the IDs of the individual frames could be mapped to the same data structure instance. I would propose using shared_ptrs and weak_ptrs to handle deallocating the data and keeping track of which entries in the map are unused.
I looked through the bmpman header a bit and noticed a few unused function. Here is a github branch with those removed: https://github.com/asarium/fs2open.github.com/tree/refactor/bmpman

 

Offline Goober5000

  • HLP Loremaster
  • Moderator
  • 214
    • Goober5000 Productions
Re: Let's talk about bmpman
Not being a graphics coder myself, I'll just sit back and observe the discussion.

I will say that if we are going to do this then it should be planned out as carefully as possible.  BMPMAN is a large system with its fingers in many places, and it was developed by a professional game studio.  We should consider the choices and tradeoffs they made in their design.

 

Offline Swifty

  • 210
  • I reject your fantasy & substitute my own
Re: Let's talk about bmpman
Eh, I feel a lot of the problems with us hitting the BMP limit is that we're just not very smart when it comes to deciding what textures need to be resident versus what doesn't need to be resident. A lot of that is outside the internals of bmpman and more of determining what needs bm_release to be called on it.

The other problem is that the way we do animations, especially for effects, is kind of ridiculous. Modern game engines atlas most of their effect animations. Here, we're still using series of individual textures. Most of Wings of Dawn's textures come from small effect animations and had Spoon had the capability to shove all his effects into atlases, we wouldn't be having this conversation right now (Maybe later once a bigger structural issue with BMPMAN is revealed)

Regardless, I feel that if we are to scrap BMPMAN for something else, we need to keep in mind of how modern GPUs handle textures. Even now we're using depth render textures, stencil targets, floating point textures, texture buffer objects, and array textures. The scene textures and g-buffers that are used for post processing, deferred rendering, and soft particle rendering are completely divorced from BMPMAN. I'm trying to think of ways to pull them back in but it's going to require some thinking on my part. If any actionable discussion comes from this, I'm going to suggest you guys wait until I get a good idea of what needs to be done.
« Last Edit: April 03, 2015, 11:45:05 am by Swifty »

 

Offline z64555

  • 210
  • Self-proclaimed controls expert
    • Minecraft
    • Steam
Re: Let's talk about bmpman
To help with the effort, I've doxy'd the bmpman.h file. This is still a WIP, as it needs some organization as well as some other lookovers.

[attachment deleted by nobody]
Secure the Source, Contain the Code, Protect the Project
chief1983

------------
funtapaz: Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Juche.
z64555: s/J/Do
BotenAlfred: <funtapaz> Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Douche.

 

Offline Echelon9

  • 210
Re: Let's talk about bmpman
After a discussion on #scp here is what we came up with:
  • Use an int size_t for the ID and increment it every time we need a new slot (animations are created in one call so for a frame i the ID is startID + i)
...

size_t plz.

 

Offline m!m

  • 211
Re: Let's talk about bmpman
size_t plz.
That would be better but bmpman currently uses ints as handles with -1 being an invalid handle. If we were to change that to a size_t we would need to check every usage of a bmpman handle to make sure that the new "invalid" value is used and not -1.

 

Offline Echelon9

  • 210
Re: Let's talk about bmpman
Ah yeah. Perhaps a time and place to separately reconsider the engine's use of in line error reporting via those -1 handles.

 

Offline The E

  • He's Ebeneezer Goode
  • Moderator
  • 213
  • Nothing personal, just tech support.
    • Steam
    • Twitter
Re: Let's talk about bmpman
size_t plz.
That would be better but bmpman currently uses ints as handles with -1 being an invalid handle. If we were to change that to a size_t we would need to check every usage of a bmpman handle to make sure that the new "invalid" value is used and not -1.

This is why I specified that no handle should be invalid once assigned. Obviously error checking has to happen, but I would rather turn an error here into an abort for the entire engine.
If I'm just aching this can't go on
I came from chasing dreams to feel alone
There must be changes, miss to feel strong
I really need lifе to touch me
--Evergrey, Where August Mourns

 

Offline m!m

  • 211
Re: Let's talk about bmpman
The problem at hand is that when the engine fails to load a bitmap (e.g. it doesn't exist) the caller should know that. Trying to load a bitmap too see if it exists is a common practice in the engine which shouldn't cause an abort.

 

Offline The E

  • He's Ebeneezer Goode
  • Moderator
  • 213
  • Nothing personal, just tech support.
    • Steam
    • Twitter
Re: Let's talk about bmpman
Right, and I think those two things should be disentangled. Checking whether something exists should be done through cfile alone, actually loading things should be handled by bmpman.
If I'm just aching this can't go on
I came from chasing dreams to feel alone
There must be changes, miss to feel strong
I really need lifе to touch me
--Evergrey, Where August Mourns

 

Offline m!m

  • 211
Re: Let's talk about bmpman
Doing it through cfile would not be sufficient as bmpman checks for multiple extensions. It might be a good idea to add a bm_exists function which would use the same lookup mechanisms as bm_load.

 

Offline z64555

  • 210
  • Self-proclaimed controls expert
    • Minecraft
    • Steam
Re: Let's talk about bmpman
OK, completed. Split off the bm_create example to its own file (which shouldn't be included in the makefiles) and organized bmpman.cpp quite a bit.

Also upgraded the #defines into a properenum, and mark some things I spotted with @todo in the doxy. Hopefully this'll help.  :)


If there are no objections, I'd like to commit the patch to trunk, seeing as there isn't any major changes to bmpman's operation.

[attachment deleted by nobody]
Secure the Source, Contain the Code, Protect the Project
chief1983

------------
funtapaz: Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Juche.
z64555: s/J/Do
BotenAlfred: <funtapaz> Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Douche.

 

Offline z64555

  • 210
  • Self-proclaimed controls expert
    • Minecraft
    • Steam
Re: Let's talk about bmpman
Doxy committed as of r11303.

I defined the EXAMPLE_PATH in doxyfile to enable the @example command to work properly. It hasn't caused problems on my end, but let me know if it does.
Secure the Source, Contain the Code, Protect the Project
chief1983

------------
funtapaz: Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Juche.
z64555: s/J/Do
BotenAlfred: <funtapaz> Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Douche.

 

Offline Echelon9

  • 210
Re: Let's talk about bmpman
This refactoring and documentation also provides an opportunity to consider unit testing the interfaces exposed by BMPMAN. Todo so would enhance the productivity of any subsequent refactoring.

Thoughts?

 

Offline The E

  • He's Ebeneezer Goode
  • Moderator
  • 213
  • Nothing personal, just tech support.
    • Steam
    • Twitter
Re: Let's talk about bmpman
All of my yes.
If I'm just aching this can't go on
I came from chasing dreams to feel alone
There must be changes, miss to feel strong
I really need lifе to touch me
--Evergrey, Where August Mourns

 

Offline m!m

  • 211
Re: Let's talk about bmpman
Unit tests would be a great improvement but writing unit tests may not be as easy as it sounds. I have added tests for a few cases while replacing the Lua API and I have encountered numerous issues as FSO isn't designed with unit tests in mind.
Here is my current unit testing setup if you want to take a look at it: https://github.com/asarium/fs2open.github.com/tree/feature/luabind/test

 

Offline Spoon

  • 212
  • ヾ(´︶`♡)ノ
Re: Let's talk about bmpman
Bumping this because JAD has started to run into the bmpman limits too!
Coders, bring down this wall!
Urutorahappī!!

[02:42] <@Axem> spoon somethings wrong
[02:42] <@Axem> critically wrong
[02:42] <@Axem> im happy with these missions now
[02:44] <@Axem> well
[02:44] <@Axem> with 2 of them

 

Offline Spoon

  • 212
  • ヾ(´︶`♡)ノ
Re: Let's talk about bmpman
Ded forum, ded scp.
Nothing seems to get done anymore :(
Urutorahappī!!

[02:42] <@Axem> spoon somethings wrong
[02:42] <@Axem> critically wrong
[02:42] <@Axem> im happy with these missions now
[02:44] <@Axem> well
[02:44] <@Axem> with 2 of them

 

Offline z64555

  • 210
  • Self-proclaimed controls expert
    • Minecraft
    • Steam
Re: Let's talk about bmpman
Ded forum, ded scp.
Nothing seems to get done anymore :(

I blame the long work hours. (btw, stuff is still being done, but it appears we lost a bit of focus on some things)
Secure the Source, Contain the Code, Protect the Project
chief1983

------------
funtapaz: Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Juche.
z64555: s/J/Do
BotenAlfred: <funtapaz> Hunchon University biologists prove mankind is evolving to new, higher form of life, known as Homopithecus Douche.