Hard Light Productions Forums

Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: The E on April 03, 2015, 05:27:26 am

Title: Let's talk about bmpman
Post by: The E on April 03, 2015, 05:27:26 am
Does anyone actually like that thing? I know I don't, and it seems we're starting to run into its limitations (http://www.hard-light.net/forums/index.php?topic=74794.msg1781461#msg1781461). 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.
Title: Re: Let's talk about bmpman
Post by: m!m on April 03, 2015, 08:21:10 am
After a discussion on #scp here is what we came up with:
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
Title: Re: Let's talk about bmpman
Post by: Goober5000 on April 03, 2015, 10:38:36 am
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.
Title: Re: Let's talk about bmpman
Post by: Swifty on April 03, 2015, 11:41:21 am
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.
Title: Re: Let's talk about bmpman
Post by: z64555 on April 05, 2015, 12:45:21 am
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]
Title: Re: Let's talk about bmpman
Post by: Echelon9 on April 05, 2015, 03:50:40 am
Quote from: m!m on April 03, 2015, 08:21:10 am
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.
Title: Re: Let's talk about bmpman
Post by: m!m on April 05, 2015, 03:58:14 am
Quote from: Echelon9 on April 05, 2015, 03:50:40 am
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.
Title: Re: Let's talk about bmpman
Post by: Echelon9 on April 05, 2015, 04:04:04 am
Ah yeah. Perhaps a time and place to separately reconsider the engine's use of in line error reporting via those -1 handles.
Title: Re: Let's talk about bmpman
Post by: The E on April 05, 2015, 04:16:23 am
Quote from: m!m on April 05, 2015, 03:58:14 am
Quote from: Echelon9 on April 05, 2015, 03:50:40 am
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.
Title: Re: Let's talk about bmpman
Post by: m!m on April 05, 2015, 05:11:34 am
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.
Title: Re: Let's talk about bmpman
Post by: The E on April 05, 2015, 05:17:43 am
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.
Title: Re: Let's talk about bmpman
Post by: m!m on April 05, 2015, 06:39:08 am
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.
Title: Re: Let's talk about bmpman
Post by: z64555 on April 06, 2015, 06:34:38 pm
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]
Title: Re: Let's talk about bmpman
Post by: z64555 on April 13, 2015, 06:12:28 am
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.
Title: Re: Let's talk about bmpman
Post by: Echelon9 on April 30, 2015, 11:46:36 pm
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?
Title: Re: Let's talk about bmpman
Post by: The E on May 01, 2015, 12:55:19 am
All of my yes.
Title: Re: Let's talk about bmpman
Post by: m!m on May 01, 2015, 04:05:35 am
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
Title: Re: Let's talk about bmpman
Post by: Spoon on July 19, 2015, 09:46:19 pm
Bumping this because JAD has started to run into the bmpman limits too!
Coders, bring down this wall!
Title: Re: Let's talk about bmpman
Post by: Spoon on August 11, 2015, 01:57:36 pm
Ded forum, ded scp.
Nothing seems to get done anymore :(
Title: Re: Let's talk about bmpman
Post by: z64555 on August 11, 2015, 04:12:07 pm
Quote from: Spoon on August 11, 2015, 01:57:36 pm
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)
Title: Re: Let's talk about bmpman
Post by: niffiwan on August 24, 2015, 05:12:09 am
So, after talking with Axem on IRC today it seems that a *single* JAD mission is exceeding the current BMPMAN limits.  He's got 2800+ slots filled with talking head frames (:nervous:), so being more aggressive with unloading bitmaps isn't going to help. I'd also wager that the increased slot fragmentation of unloading/loading moderate-to-lengthy animations means that across multiple missions the BMPMAN limit is going to be hit well below the current 4750 slots.

Anyway, so maybe the focus should be on implementing texture/animation atlasing (http://http.download.nvidia.com/developer/NVTextureSuite/Atlas_Tools/Texture_Atlas_Whitepaper.pdf) (which I 1st heard of here in this thread, from Swifty!) independently of messing with BMPMAN? i.e. as I understand it so far, we could have a single mega-texture stored in a single BMPMAN slot rather than animation frames strewn across heaps of slots.

(Am I just being Captain Obvious here?)

(also, I thought I should have a look at this when I realised that apng support isn't going to fix anyone's BMPMAN slot issues, at least, not if I follow the current animations framework within FSO)
Title: Re: Let's talk about bmpman
Post by: m!m on August 24, 2015, 05:16:08 am
We would still need to keep track of where a single frame is in the atlas which would consume a bmpman slot so this probably won't fix our problems...
Title: Re: Let's talk about bmpman
Post by: z64555 on August 24, 2015, 07:17:59 am
Nah, I'm thinking we should update how animations are stored in RAM, instead needing a slot for every frame in the animation, we'd have a single slot for the start of the animation and then reference the other frames through that one.
Title: Re: Let's talk about bmpman
Post by: m!m on August 24, 2015, 07:21:21 am
It could help but it's not a permanent solution.
Also, maybe I'm missing something but shouldn't animations never be stored in RAM but only in the video RAM? I don't see much point in keeping them in the normal RAM where we can't really do much with them.
Title: Re: Let's talk about bmpman
Post by: niffiwan on August 24, 2015, 07:38:38 am
Wouldn't you want them in RAM to avoid reading from disk when you need them? I also figure that loading them only into video memory for the duration of a mission is likely to exhaust it (presuming that model textures will tend to be much larger than most animations?)

Also, why would we use bmpman to store which location the frame is within an atlas? Couldn't that info be stored elsewhere? i.e. generic anim's already store the current frame in the generic_anim struct.
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on August 24, 2015, 10:29:10 am
The only problem is that if you change animations to not consume multiple BMPMAN slots, you break every script that uses animation frames. On the other hand, the benefits would probably outweigh the cost there.
Title: Re: Let's talk about bmpman
Post by: mjn.mixael on August 24, 2015, 10:41:08 am
Are there any major scripts that would be affected? I'm wondering about the explosion effects in the MediaVPs, for example. As long as there is an alternate solution for those scripts and we have an idea of which ones to fix up... we could deal with the fallout pretty quickly.
Title: Re: Let's talk about bmpman
Post by: Axem on August 24, 2015, 11:01:04 am
I think the explosion effects in that script are just particles, so I don't think they would be affected (not sure, I'd have to look at the script when I get home).

But with regards to animations and scripts, is that really a problem? The smart way to do that right now is with gr.loadTexture(), with an argument to load as an animation and then you draw the frames and change the frame index every frame. The script never actually directly touch the raw files, even with effs (and as such, the script writer needs to make sure he/she unloads the texture!). Or would it break even then?

Unless you mean scripts that intentionally use a single frame of an animation like an image from a larger eff? Or I might be misunderstanding something...
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on August 24, 2015, 11:23:19 am
Quote from: Axem on August 24, 2015, 11:01:04 am
But with regards to animations and scripts, is that really a problem? The smart way to do that right now is with gr.loadTexture(), with an argument to load as an animation and then you draw the frames and change the frame index every frame. The script never actually directly touch the raw files, even with effs (and as such, the script writer needs to make sure he/she unloads the texture!). Or would it break even then?

Unless you mean scripts that intentionally use a single frame of an animation like an image from a larger eff? Or I might be misunderstanding something...
I... hmm. For some reason I thought scripts were using BMPMAN slots directly, but a closer look at the only example I had on hand shows it using the animation returned by loadTexture(), so I guess they'd actually be fine.
Title: Re: Let's talk about bmpman
Post by: niffiwan on August 25, 2015, 06:44:03 am
After more investigation it seems that talking head EFFs only use two BMPMAN slots each, at least when they're being rendered. One slot is the 1st frame, one is the current frame. On the other hand, it looks like FSO is reserving enough BMPMAN slots for every frame; whether this is actually a problem or not I'm not 100% sure, it needs some more investigation.

As part of this investigation I've found a way to get a list of all BMPMAN slots used per mission; just add this to your debug_filter.cfg and it's dumped to the log just before the mission starts.

Code: [Select]
+BMP DEBUG

(and you probably want the usual suspects as well)
Code: [Select]
+General
+Warning
+Error
+BMP DEBUG

Exactly how much of this output is "lying" is yet to be seen, "best case" maybe all the EFFs (or at least the talking heads) only need two frames... wouldn't that be an easy win! :)
Title: Re: Let's talk about bmpman
Post by: Axem on August 25, 2015, 08:34:51 am
Bmpman. ¯\_(ツ)_/¯

Am I rite coders?

(Seriously though, if the best case is true, I will be very very happy)
Title: Re: Let's talk about bmpman
Post by: Spoon on September 06, 2015, 04:19:17 pm
Bmpman is so weak, it needs to lift all these bitmaps! GET FIT.

JAD and WoD can't function as long as you remain a scrawny little excuse for a line of code! GO TO THE GYM, BMPMAN.
Title: Re: Let's talk about bmpman
Post by: niffiwan on September 06, 2015, 05:16:37 pm
I think I've got headanis (only) using only 3 bmpman slots each; not quite finished but close.  Here's my TODO list:

Code: [Select]
[X] only look for 3 frames to hold eff-anim instead of all frames (NEEDS TESTING)
[ ] reset 3rd frame back to _0001 after finishing playing anim (so we don't keeping using up slots)
[ ] update unload/release functions to handle the 3-frame anis
[x] see if we can release headani's between missions (don't just unload them) (looks like they are already released)
[x] correctly deal with EFFs that don't have all the frames AND are streaming - cap frames to "reset" to 3 (NEEDS TESTING)
[ ] deal with someone trying to use an EFF for streaming AND non-streaming (not sure where to store the "type" data just yet...)
[ ] Test Axem's headani script
[ ] Remove all temp debug code

Note that I'm pretty sure I can't help the effects animations (at least not easily), in the new deferred rendered these are handled differently to headanis so the same easy win doesn't apply to them. Atlasing could help, but that's a far bigger & more complicated change.
Title: Re: Let's talk about bmpman
Post by: Spoon on September 06, 2015, 06:50:54 pm
You are a star, niffi.

Be sure to let me know if you need anything tested, you know where to find me!
Title: Re: Let's talk about bmpman
Post by: m!m on September 12, 2015, 04:25:28 pm
I've been thinking about a complete rewrite of bmpman in order to fix these issues once and for all (until other issues arise :nervous:) but before doing any serious work it would be best to have some sort of specification what bmpman should be able to do. Here is what I have come up with so far, feedback and additions are obviously welcome:

Title: Re: Let's talk about bmpman
Post by: Echelon9 on September 14, 2015, 12:17:15 am
Quote from: m!m on September 12, 2015, 04:25:28 pm
  • Support for S3TC

Can we also add support for ASTC (http://www.phoronix.com/scan.php?page=news_item&px=MTE1NDk) (see also here for technical comparison (https://www.opengl.org/discussion_boards/showthread.php/185568-ASTC-on-the-desktop?p=1263807&viewfull=1#post1263807))? This is Adaptive Scalable Texture Compression and is a lossy, block-based algorithm originally designed by ARM and importantly is royalty-free.

Native hardware support is starting to appear on modern graphics chipsets, e.g. Intel's Skylake (https://www.phoronix.com/scan.php?page=news_item&px=ASTC-Skylake-Intel-Mesa).
Title: Re: Let's talk about bmpman
Post by: niffiwan on September 14, 2015, 12:52:04 am
For animations I really think we should use atlasing; WoD has hit issues from all the effects anims chewing up the available slots. One altas per animation could be the simplest way to do this; but it would be better to have multiple anims per atlas, e.g. the approx 6x used in the mediavps_2014 explosions/flashy-death scripts could be atlased as one image. Naturally this'll require a bunch of changes outside BMPMAN in order to track & process the uv coords (I think that's the correct term) for each frame.

Or have the option to load animations in two modes, either "fat n+i" mode, or streaming mode with a single slot used and the textures rebound for each frame through the one slot.

edit: ACK - of course limits only matter if we hit perf issues from having too many slots in-use at once and that'll depend on the underlying data structures (see Chandler Carruth on high-perf C++ data structures). On rereading your list I realise that the number of slots used may not matter. Still - for faster GPU performance I think atlasing is the answer, not more/unlimited slots (but it may be able to be separated from this proposed change).
Title: Re: Let's talk about bmpman
Post by: Fury on September 14, 2015, 01:10:11 am
While ASTC has been an official extension to OpenGL since 2012, it never became part of the core spec afaik. ASTC became part of Direct3D spec only in 11.3 and 12. No word on Vulkan regarding ASTC, but it is safe to assume ASTC is in the spec. This pretty much means that only GPUs that officially supports full spec of D3D 11.3 and 12 or Vulkan are guaranteed to have hardware support for ASTC textures.

Intel Skylake supports D3D 12, so obviously it would also support ASTC. Neither NVIDIA or AMD has yet to release next-gen GPU's that have full D3D 12 support, partial support does not guarantee ASTC support and there simply is no reliable information on the subject.
Title: Re: Let's talk about bmpman
Post by: m!m on September 14, 2015, 05:39:16 am
Quote from: Echelon9 on September 14, 2015, 12:17:15 am
Can we also add support for ASTC (http://www.phoronix.com/scan.php?page=news_item&px=MTE1NDk) (see also here for technical comparison (https://www.opengl.org/discussion_boards/showthread.php/185568-ASTC-on-the-desktop?p=1263807&viewfull=1#post1263807))? This is Adaptive Scalable Texture Compression and is a lossy, block-based algorithm originally designed by ARM and importantly is royalty-free.
ASTC is still not widely supported by the hardware so most times we would need to decompress the texture in software and then send the decompressed data to the GPU. Bmpman can obviously be designed in such a way that different compression methods can be added later and I intend to do just that.

Re Atlasing: Would the modder have to create the texutre atlas manually and then just tell FSO the layout or should FSO generate the atlas by itself? OpenGL (and probably Vulkan) has support for "Array Textures" which are similar to texture atlases but apparently avoid some of the problems. If we use array textures then FSO needs to be able to separately access the data of the individual frames and that would mean that the modder keeps the data in separate images and FSO only combines them via the OpenGL/Vulkan functions.

I don't think that texture arrays should be done as part of the bmpman rewrite because that's something the OpenGL backend has to handle, not bmpman. I will still try to make sure bmpman can provide all the necessary information for building the texture atlas.

I updated the list with to reflect the recent comments.
Title: Re: Let's talk about bmpman
Post by: Spoon on September 14, 2015, 10:05:57 am
Quote from: m!m on September 14, 2015, 05:39:16 am
Re Atlasing: Would the modder have to create the texutre atlas manually and then just tell FSO the layout or should FSO generate the atlas by itself?
Considering I don't even have the vaguest of clues what that even means, and considering the sheer list of stuff we already have to deal with during modding, my initial response to this is "Dear FRED, not more **** to deal with, please let FSO do everything for us!"
Title: Re: Let's talk about bmpman
Post by: mjn.mixael on September 14, 2015, 10:16:28 am
Quote from: Spoon on September 14, 2015, 10:05:57 am
Quote from: m!m on September 14, 2015, 05:39:16 am
Re Atlasing: Would the modder have to create the texutre atlas manually and then just tell FSO the layout or should FSO generate the atlas by itself?
Considering I don't even have the vaguest of clues what that even means, and considering the sheer list of stuff we already have to deal with during modding, my initial response to this is "Dear FRED, not more **** to deal with, please let FSO do everything for us!"

Yeah.. this.
Title: Re: Let's talk about bmpman
Post by: m!m on September 14, 2015, 11:20:06 am
That would favor using texture arrays which I also prefer but that will probably not happen until we have a Vulkan rendering engine because I'm not going near the OpenGL code :p
Title: Re: Let's talk about bmpman
Post by: z64555 on September 14, 2015, 12:47:05 pm
Texture atlassing involves taking all the textures used by the engine and saving them as one big texture, thereby making it easy for the GPU to load because it doesn't have to hunt through the OS's filesystem to find every frame.

In regards to .EFF's, this would most likely mean having FSO or an external tool assemble the involved frames into a new texture no larger than 4096x4096, or whatever the maximum texture size the GPU supports. Although that's likely to cause issues with memory hogging if you did the maximum. That said, if speed and efficiency is the goal for animated textures, then an animation format such as .APNG, .ANI, .GIF would be better than trying to load individual textures. I personally view the .EFF's as a pre-production item rather than a final product.
Title: Re: Let's talk about bmpman
Post by: The E on September 14, 2015, 01:56:19 pm
Both solutions would have to be implemented engine-side, because there's no way in hell to get all the old animations converted.

Personally, I would much rather prefer using texture arrays, as they are a bit easier to implement (and do not involve the creation of massive intermediate textures with god knows how many pixels wasted).
Title: Re: Let's talk about bmpman
Post by: m!m on September 14, 2015, 02:54:24 pm
Texture arrays can also be used to render submodels using multiple textures with only one draw call.

Also, exactly what problem should the texture atlases solve? As far as I can tell they are only really useful if different textures are used together which is pretty much the opposite to how head anis work where we only need one animation frame each frame. I guess they could be useful for particle effects as multiple frames of the same animation are typically used together.
Title: Re: Let's talk about bmpman
Post by: niffiwan on September 14, 2015, 06:27:15 pm
yeah, atlasing / arrays would be most useful for effects where there's the potential to have multiple instances displaying at the same time; e.g. impact anims, explosions, particles, etc.  Head ani's less so since as you say there's only one playing at a time (once you eliminate the issue of them using too many bmpman slots, or remove the slot limit)
Title: Re: Let's talk about bmpman
Post by: niffiwan on September 15, 2015, 06:51:18 am
Quote from: niffiwan on September 06, 2015, 05:16:37 pm
I think I've got headanis (only) using only 3 bmpman slots each; not quite finished but close.  Here's my TODO list:

Code: [Select]
[X] only look for 3 frames to hold eff-anim instead of all frames (NEEDS TESTING)
[ ] reset 3rd frame back to _0001 after finishing playing anim (so we don't keeping using up slots)
[ ] update unload/release functions to handle the 3-frame anis
[x] see if we can release headani's between missions (don't just unload them) (looks like they are already released)
[x] correctly deal with EFFs that don't have all the frames AND are streaming - cap frames to "reset" to 3 (NEEDS TESTING)
[ ] deal with someone trying to use an EFF for streaming AND non-streaming (not sure where to store the "type" data just yet...)
[ ] Test Axem's headani script
[ ] Remove all temp debug code

Note that I'm pretty sure I can't help the effects animations (at least not easily), in the new deferred rendered these are handled differently to headanis so the same easy win doesn't apply to them. Atlasing could help, but that's a far bigger & more complicated change.

I think I found a far simpler way of "fixing" this. In short, head ani's shouldn't be loading (https://github.com/scp-fs2open/fs2open.github.com/compare/master...niffiwan:headzlessII) all the frames into bmpman slots because they're streamed. I still need to thoroughly test this before creating a PR, to make sure my assumptions are all correct.  Then again; there was no anim data loaded here until after EFF headani's were implemented by Valathil anyway! (see 48e05, aa463 & cb1f81)
Title: Re: Let's talk about bmpman
Post by: Axem on September 15, 2015, 08:07:21 am
*Axem goes to test right away

Initial findings: AMAZEMENT. Missions that crashed on load before, now load. Head anis play as expected, animated textures and effects still play!

Detailed findings:
(Reference: BMPMAN limit is 4750)
MissionBefore  After 
Beta Tester Counterattack36122357
Escape from HighschoolCrash2433
...Get RevengeanceCrash2902

The first mission is a "low weight" mission in head anis, its mostly just Holley (animated) taunting talking to a Beta Tester (static)
The other two are "heavy weight" missions. "Escape..." is head ani heavy (and still doesn't have all the art done) and it goes from crashing to 50% of the slots used!
"...Get Revengeance" is a long mission with tons of weapons and effects and talking and even then its just 61% filled.

Conclusion: Amazing results.
Title: Re: Let's talk about bmpman
Post by: Spoon on September 15, 2015, 09:19:55 am
(http://i1054.photobucket.com/albums/s490/kingspoon/Junk/url_zpsqrvhhwug.jpg~original)
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on September 15, 2015, 03:07:23 pm
I wonder if this change alone would allow nuWoD missions to load.
Title: Re: Let's talk about bmpman
Post by: Spoon on September 15, 2015, 04:58:04 pm
I was wondering about that too, nuWoD's headani's are between 30 to 45 frames. So it'd save like roughly 250-350 slots on the average mission.
I don't know if that would be enough or not, the majority of slots are probably eaten up by explosion effects and the like.
Title: Re: Let's talk about bmpman
Post by: niffiwan on September 15, 2015, 06:29:23 pm
I thought the issues with nuWod occurred after multiple missions were played, one of the changes I made was to fix the streaming headani's from "leaking" slots.  i.e. each time an ani played it left a slot "in-use" that I don't think was ever cleaned up. Anyway, I thought a good acid test of the change would be to play through nuWod & see what happens, IIRC you have EFF mainhall anis, lots of effects and Axems headani script?
Title: Re: Let's talk about bmpman
Post by: niffiwan on September 18, 2015, 05:04:38 am
So, I managed to play through nuWoD last night with my latest change (https://github.com/scp-fs2open/fs2open.github.com/pull/350) (now a PR) and MAX_BITMAPS set back to 4750. I had several crashes which reset bmpman usage but I didn't have any issues with missing textures or similar. Cautiously optimistic?

Here's a record of the bitmap usage at each mission load (yes, I had to repeat a few missions quite a few times before completing them, Contravention I & II, plus those pesky Cyrvans :))
Code: [Select]
Bmpman: 3814/4750 bitmap slots in use.
Bmpman: 3545/4750 bitmap slots in use.
Bmpman: 3413/4750 bitmap slots in use.
Bmpman: 3929/4750 bitmap slots in use.
Bmpman: 3848/4750 bitmap slots in use.
Bmpman: 3750/4750 bitmap slots in use.
Bmpman: 4051/4750 bitmap slots in use.
Bmpman: 3917/4750 bitmap slots in use.
Bmpman: 3949/4750 bitmap slots in use.
Bmpman: 3849/4750 bitmap slots in use.
Bmpman: 3864/4750 bitmap slots in use.
Bmpman: 3864/4750 bitmap slots in use.
Bmpman: 3877/4750 bitmap slots in use.
Bmpman: 3878/4750 bitmap slots in use.
Bmpman: 3879/4750 bitmap slots in use.
Bmpman: 4091/4750 bitmap slots in use.
(contravention II crash)
Bmpman: 3528/4750 bitmap slots in use.
Bmpman: 3639/4750 bitmap slots in use.
Bmpman: 3791/4750 bitmap slots in use.
Bmpman: 3868/4750 bitmap slots in use.
Bmpman: 3680/4750 bitmap slots in use.
Bmpman: 3844/4750 bitmap slots in use.
Bmpman: 3891/4750 bitmap slots in use.
Bmpman: 4188/4750 bitmap slots in use.
(battle cutscene crash, skipped watching it)
Bmpman: 3752/4750 bitmap slots in use.
Bmpman: 3774/4750 bitmap slots in use.
Bmpman: 3808/4750 bitmap slots in use.
Bmpman: 3822/4750 bitmap slots in use.
Bmpman: 3825/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3826/4750 bitmap slots in use.
Bmpman: 3838/4750 bitmap slots in use.
Bmpman: 3838/4750 bitmap slots in use.
Bmpman: 3839/4750 bitmap slots in use.
Bmpman: 3839/4750 bitmap slots in use.
Bmpman: 3839/4750 bitmap slots in use.
Bmpman: 3839/4750 bitmap slots in use.
Bmpman: 3839/4750 bitmap slots in use.
Bmpman: 3628/4750 bitmap slots in use.
Bmpman: 3759/4750 bitmap slots in use.

(actually, reviewing this more closely there may be some other type of slot leaking, since the load counts for Contravention II before & after the crash were significantly different... so maybe that optimism is misplaced)
Title: Re: Let's talk about bmpman
Post by: Spoon on September 18, 2015, 08:32:45 am
Hey, that's still below the limit, so definitely a success there! (As long as I don't add any new explosion effects :p )
Title: Re: Let's talk about bmpman
Post by: Axem on September 18, 2015, 06:39:39 pm
I might have mentioned this before, but I'll throw it out again:

Swifty mentioned awhile ago that the scripting system's bitmap unloading wasn't using the right method to permanently unload bitmaps between state changes or something. So bitmaps used in screens like the journal or visual novel scripts were still using bmpman slots when they're not supposed to. Maybe that is causing some of the slot leakage?
Title: Re: Let's talk about bmpman
Post by: niffiwan on September 18, 2015, 07:12:39 pm
Well, the texture.unload() LUA function calls bm_release; which is the correct function to completely free a bmp slot.

Code: [Select]
ADE_FUNC(unload, l_Texture, NULL, "Unloads a texture from memory", NULL, NULL)
{
int *idx;

if(!ade_get_args(L, "o", l_Texture.GetPtr(&idx)))
return ADE_RETURN_NIL;

if(!bm_is_valid(*idx))
return ADE_RETURN_NIL;

bm_release(*idx);

//WMC - invalidate this handle
*idx = -1;

return ADE_RETURN_NIL;
}

Also, these seems to be a garbage collection function for textures which will also bm_release them? I'm not sure if this is called at mission end or game end though.

Code: [Select]
ADE_FUNC(__gc, l_Texture, NULL, "Auto-deletes texture", NULL, NULL)
{
int idx;

if(!ade_get_args(L, "o", l_Texture.Get(&idx)))
return ADE_RETURN_NIL;

// Note: due to some unknown reason, in some circumstances this function
// might get called even for handles to bitmaps which are actually still in
// use, and in order to prevent that we want to double-check the load count
// here before unloading the bitmap. -zookeeper
if(idx > -1 && bm_is_valid(idx) && bm_bitmaps[bm_get_cache_slot(idx, 0)].load_count < 1)
bm_release(idx);

return ADE_RETURN_NIL;
}

Having said that, I can't see anywhere in the nuWoD VN/Journal scripts where the lua unload function is being called?

Code: [Select]
$ grep -iR unload Tables/ Scripts/
Tables/mv_exp-sct.tbm:   arr_D_eff[j]:unload()
Tables/proBox-sct.tbm: self.Head.Filename:unload()
Title: Re: Let's talk about bmpman
Post by: Axem on September 18, 2015, 07:59:33 pm
Right, I should be more clear. That setup works fine, 100%! (I've even done it that way on some new stuff and confirmed the BMPMAN slots get released properly)

The issue is with gr.drawBitmap() using a filename (which is what I was using 99% of the time) instead of a texture loaded with gr.loadTexture(). When gr.drawBitmap() with a filename gets called, the bitmap gets thrown into bmpman as expected (and also a ScriptImages vector). But then in game_leave_state() (freespace.cpp around line 5617), Script_system.UnloadImages() gets called to bm_unload() all of the images that were called with the scripting system. (Not bm_release() which seems to be the more correct way to do it?)

That's what I had sort of picked up anyway as a semi-literate coder reader. ;)
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on September 18, 2015, 08:06:47 pm
Quote from: Axem on September 18, 2015, 07:59:33 pm
Right, I should be more clear. That setup works fine, 100%! (I've even done it that way on some new stuff and confirmed the BMPMAN slots get released properly)

The issue is with gr.drawBitmap() using a filename (which is what I was using 99% of the time) instead of a texture loaded with gr.loadTexture(). When gr.drawBitmap() with a filename gets called, the bitmap gets thrown into bmpman as expected (and also a ScriptImages vector). But then in game_leave_state() (freespace.cpp around line 5617), Script_system.UnloadImages() gets called to bm_unload() all of the images that were called with the scripting system. (Not bm_release() which seems to be the more correct way to do it?)

That's what I had sort of picked up anyway as a semi-literate coder reader. ;)
Well, that should be as trivial as replacing the bm_unload() call with a bm_release() call.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 12, 2015, 11:12:09 am
Quote from: Bryan See on December 12, 2015, 10:51:22 am
I've tested this 64-bit build against the eleventh mission of the mod Shattered Stars, and I've got the BMPMAN assertion:

Code: [Select]
Assert: bm_bitmaps[bitmapnum].handle == handle
File: bmpman.cpp
Line: 683
Invalid bitmap handle 561502 passed to bm_get_info().
This might be due to an invalid animation somewhere else.


ntdll.dll! ZwWaitForSingleObject + 12 bytes
KERNELBASE.dll! WaitForSingleObject + 18 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! SCP_DumpStack + 354 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! WinAssert + 293 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! bm_get_info + 180 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! HudGaugeWeaponEnergy::render + 2820 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! hud_render_gauges + 555 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! hud_render_all + 37 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! game_render_hud + 150 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! game_frame + 957 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! game_do_frame + 231 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! game_do_state + 403 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! gameseq_process_events + 232 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! game_main + 787 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! WinMain + 328 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! invoke_main + 30 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! __scrt_common_main_seh + 346 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! __scrt_common_main + 13 bytes
fs2_open_3_7_3_SSE2-DEBUG.exe! WinMainCRTStartup + 8 bytes
KERNEL32.DLL! BaseThreadInitThunk + 36 bytes
ntdll.dll! RtlUnicodeStringToInteger + 595 bytes
ntdll.dll! RtlUnicodeStringToInteger + 542 bytes

I think BMPMAN should use only one same bitmap, instead of multiple same bitmaps. The same thing goes for animations.

And on-the-fly file/cache streaming should be implemented as well.
Title: Re: Let's talk about bmpman
Post by: The E on December 12, 2015, 11:33:04 am
Go do that then.

Seriously, Bryan, at this point you should know that we are aware of the problems and know the possible solutions; Unless you have actual contributions to make, your sort of "This feature/thing/whatever should be used" posts are somewhere between useless and irritating.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 13, 2015, 10:35:37 am
And I started doing (https://github.com/bryansee/fs2open.github.com/commit/a75c0d4b3983d3f3fcf7ab8729f05418cfddd11b) it.

First of all, I've cleaned up all of the unused ones, and add a barebones ID code to identify the bitmap IDs.
Title: Re: Let's talk about bmpman
Post by: The E on December 13, 2015, 10:54:42 am
Why did you remove all the doxygen comments? It seems like you have reverted the bmpman files to an earlier revision, was that on purpose? And if yes, what was the purpose?
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on December 13, 2015, 11:00:16 am
If GitHub won't show the diff of bmpman.cpp due to being too long, I've pastebinned a diff of the commit here (http://pastebin.com/7NwX8MG7).
Title: Re: Let's talk about bmpman
Post by: The E on December 13, 2015, 11:03:56 am
Yeah, that's definitely an earlier version of that file.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 13, 2015, 11:09:45 am
I've started working on it, but where the hell is the boost library? Is it included in the FS2 SCP code repository in GitHub? Or do I have to download a latest one?

EDIT: This is a response to The E's earlier post.

Quote from: The E on December 13, 2015, 10:54:42 am
Why did you remove all the doxygen comments? It seems like you have reverted the bmpman files to an earlier revision, was that on purpose? And if yes, what was the purpose?

To answer the question, I'm trying to remove the unused functions, as m!m stated on the second post. Yes. I'm trying to make the bitmap entry struct an unordered map from Boost to apply that ID (an incrementing integer).

EDIT #2: To try to streamline the code and make it efficient, reducing the size of the resulting binary file (compiled form) smaller.
Title: Re: Let's talk about bmpman
Post by: The E on December 13, 2015, 12:16:47 pm
That does not explain why you removed the doxygen comments and altered a few function prototypes without good reason. For example, we changed bm_load to accept an SCP_string as input, why did you change that?

Again, all the changes you made seem to consist of:
-Adding an id field to the bm_entry and bitmap structs
-Reverting the bmpman files and headers to a previous version, one before the addition of doxygen comments.

The latter is the one you need to justify.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 13, 2015, 12:58:46 pm
Sorry, the id denotes a bitmap ID that you are talking about. The ID is meant to be an increasing integer consistent for the lifetime of the program.

As for the latter one, we want to remove a few unused function as spotted by m!m.

Quote from: m!m on April 03, 2015, 08:21:10 am
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

Should I revert back and make changes from there without removing doxygen comments?
Title: Re: Let's talk about bmpman
Post by: The E on December 13, 2015, 01:12:18 pm
First of all, if you want to remove unused functions, then that's a separate thing from introducing a new feature.
Second, removing unused functions is not as high a priority as fixing things.
Third, if you do decide that cleanup is in order, you need to start with the current version of the files you wish to clean up, not one from months and months ago.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 13, 2015, 01:34:15 pm
OK, noted. I'll start doing it right away.
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on December 15, 2015, 08:24:31 am
So, diffing with the pre-doxygen commit and stripping out the whitespace changes leads to this significantly cut-down diff: http://pastebin.com/5jbkg1CQ

The only effect of these changes (ignoring the way you've reverted bmpman.cpp and bmpman.h to earlier versions) is to waste RAM on unused "id" fields, the point of which (given the already-extant signature and handle values) I'm still not clear on.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 15, 2015, 10:07:09 am
Okay, here's the commit (https://github.com/bryansee/fs2open.github.com/commit/3b50d0045db1a90ef8d22b41b4ef2d22a7617d5b) that included restored doxygen comments, and the addition of id in some parts of bitmap handling of std::unordered_map include in the pstypes.h file. Of course, the id has to be incrementally and consistent with the lifetime of the program, and has to handle animation frames separately, as both The E and m!m pointed out.

OK, I would like to turn bitmap list into data structures via std::unordered_map.
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on December 15, 2015, 10:43:45 am
std::unordered_map is not worth using due to its poor CPU cache performance. In addition, you should squash those two commits so that the history isn't cluttered with reverting and then re-reverting the entire file.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 15, 2015, 11:22:44 am
I've squashed these two commits already on GitHub. Are there any tips for me to start working on BMPMAN. Where do I start?

I would like to introduce a program lifetime consistent id integer which increments (with animations being handled with id + startId), as The E started in the very first post.
Title: Re: Let's talk about bmpman
Post by: AdmiralRalwood on December 15, 2015, 12:01:54 pm
Quote from: Bryan See on December 15, 2015, 11:22:44 am
I've squashed these two commits already on GitHub.
No you haven't; they're clearly still two separate commits.

Quote from: Bryan See on December 15, 2015, 11:22:44 am
Are there any tips for me to start working on BMPMAN. Where do I start?
Well, I'd recommend not starting with BMPMAN, but if you insist, I'd make sure you understand the existing code before you try modifying it, since you still haven't explained what purpose this ID value serves that isn't served by the existing handle and signature values.

Additionally, you seem to have removed the ending newline from every file you touched for no apparent reason.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 16, 2015, 08:20:50 am
OK, I'm beginning to understand how BMPMAN works.

When FS2_OPEN starts, the function that BMPMAN usually starts is bm_init(). All sizes of bitmap info will be listed on debug, and free slots up to the maximum number of bitmaps will be made. Bitmap is loaded or created, filling the slots. When it is closed, bitmaps are freed.

Code: [Select]
void bm_init() {
int i;

mprintf(("Size of bitmap info = " SIZE_T_ARG " KB\n", sizeof(bm_bitmaps) / 1024));
mprintf(("Size of bitmap extra info = " SIZE_T_ARG " bytes\n", sizeof(bm_extra_info)));

if (!bm_inited) {
bm_inited = 1;
atexit(bm_close);
}

for (i = 0; i<MAX_BITMAPS; i++) {
bm_bitmaps[i].filename[0] = '\0';
bm_bitmaps[i].id = 0;
bm_bitmaps[i].type = BM_TYPE_NONE;
bm_bitmaps[i].comp_type = BM_TYPE_NONE;
bm_bitmaps[i].dir_type = CF_TYPE_ANY;
bm_bitmaps[i].info.user.data = NULL;
bm_bitmaps[i].mem_taken = 0;
bm_bitmaps[i].bm.data = 0;
bm_bitmaps[i].bm.palette = NULL;
bm_bitmaps[i].info.ani.eff.type = BM_TYPE_NONE;
bm_bitmaps[i].info.ani.eff.filename[0] = '\0';
#ifdef BMPMAN_NDEBUG
bm_bitmaps[i].data_size = 0;
bm_bitmaps[i].used_count = 0;
bm_bitmaps[i].used_last_frame = 0;
bm_bitmaps[i].used_this_frame = 0;
#endif
bm_bitmaps[i].load_count = 0;

gr_bm_init(i);

bm_free_data(i);  // clears flags, bbp, data, etc
}
}
This id field should now be a size_t, according to Echelon9. It is supposed to be incrementing, with animations being handled with frames at IDs id + startId.

The reason why I started doing is that we're starting to run into its limitations while developing my mod Shattered Stars.
Title: Re: Let's talk about bmpman
Post by: The E on December 16, 2015, 08:47:29 am
Are you sure you understood what the problem is and how what was discussed in this thread is supposed to address it? Please answer the question in your own words and without copy/pasting stuff from other posts in this thread.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 16, 2015, 10:53:31 am
Yes. The problem is that BMPMAN is starting to hit its limitations during large missions I've been building for my mod Shattered Stars (Battle of Endor missions, which FS2_Open and current modern decent computers can handle). The ID should be consistent with the lifetime of the program.
Title: Re: Let's talk about bmpman
Post by: The E on December 16, 2015, 11:29:00 am
Okay. You've probably understood what is happening, the more pertinent question is whether you know why this is happening. Not on a "because our content today is larger than what Volition dealt with" level, but on the "which specific design decisions are biting us now" level.

So, direct question: What do you believe is the core design flaw that we're running into?
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 16, 2015, 12:11:49 pm
The core design flaw is that it is outdated, dating back to retail, particularly the 1990s. It was made with significant bits of Descent code (referring to Descent 2 which was released in 1996) which do not fully meets the modern requirements of today's computers and devices, including those running latest OSes such as Windows 10, released in 2015. Among them is that the animation system was plagued by invalid bitmap handling due to invalid animation somewhere else - an engine-crashing error I encountered during my testing of my Battle of Endor mission developed for Shattered Stars using 64-bit large-addressing aware builds.

The maximum number of bitmaps and repetitive items are the problems we're running into.

And here's the code snippet of the partial solution:

Code: [Select]
struct bitmap_entry {
static size_t count;
public:
template<typename T>
static size_t get_id()
{
static size_t id = count++;
return id;
}
...
This gives the bitmap entry a unique ID in a more fool-proof way.
Title: Re: Let's talk about bmpman
Post by: The E on December 16, 2015, 12:50:00 pm
Quote from: Bryan See on December 16, 2015, 12:11:49 pm
The core design flaw is that it is outdated, dating back to retail, particularly the 1990s. It was made with significant bits of Descent code (referring to Descent 2 which was released in 1996) which do not fully meets the modern requirements of today's computers and devices, including those running latest OSes such as Windows 10, released in 2015. Among them is that the animation system was plagued by invalid bitmap handling due to invalid animation somewhere else - an engine-crashing error I encountered during my testing of my Battle of Endor mission developed for Shattered Stars using 64-bit large-addressing aware builds.

So yeah. You do not in fact know what the issues with bmpman are. I strongly recommend you take more time to familiarize with the code.

Here's a hint: Age has got nothing to do with it. The basic principles behind bmpman's design are still workable today, the concept behind it is as usable today as it was back in 2000.
Now, given modern coding practices, it could be made better. That's what we're trying to do here, but remaking something just because it's old is stupid when you do not know exactly why you're doing it, which in this case means knowing what the subtle issues are that cause problems.

Quote
The maximum number of bitmaps and repetitive items are the problems we're running into.

No, it's not. That's the surface level issue, and if it was the actual issue, we could just increase the number of bitmap slots and be done with it. Just like your attempts at raising the number of asteroids or changing path lengths, if it was just an issue of adjusting the value of one constant, we'd have done it long ago.
The actual problem goes deeper. And you, quite frankly, do NOT have the experience with the engine or C/C++ required to fix it.

Quote
And here's the code snippet of the partial solution:

Code: [Select]
struct bitmap_entry {
static size_t count;
public:
template<typename T>
static size_t get_id()
{
static size_t id = count++;
return id;
}
...
This gives the bitmap entry a unique ID in a more fool-proof way.

Two tasks.
One. Explain what the keyword "static" in C/C++ does, and what behaviour your specific usage here will result in.
Two. Explain what a "Template" in C++ is, and what it does in this code snippet of yours.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 16, 2015, 11:27:27 pm
Now, given modern coding practices, it could be made better. That's what we're trying to do here, but remaking something just because it's old is stupid when you do not know exactly why you're doing it, which in this case means knowing what the subtle issues are that cause problems.

Quote
The maximum number of bitmaps and repetitive items are the problems we're running into.

No, it's not. That's the surface level issue, and if it was the actual issue, we could just increase the number of bitmap slots and be done with it. Just like your attempts at raising the number of asteroids or changing path lengths, if it was just an issue of adjusting the value of one constant, we'd have done it long ago.
The actual problem goes deeper. And you, quite frankly, do NOT have the experience with the engine or C/C++ required to fix it. [/quote]

I get it, as I intend to do some code refactoring on it. As for my attempts at raising the number of asteroids (to my intended 1024, though it's raised from 256 to 512, short of that) or changing path lengths, I will re-visit these at some point in the future.

Quote from: The E on December 16, 2015, 12:50:00 pm
Two tasks.
One. Explain what the keyword "static" in C/C++ does, and what behaviour your specific usage here will result in.
Two. Explain what a "Template" in C++ is, and what it does in this code snippet of yours.

The static keyword is used to declare variables and functions at global scope, namespace scope, class scope and local scope. Static objects or variables are allocated when the program starts and is deallocated when the program ends. In this case here, an ID is initialized upon starting FSO, and it deallocated when FSO closes.

Templates are the foundation of generic programming, which involves writing code in a way that is independent of any particular type. Here in my code snippet, type is a placeholder name for a data type used by the function. This name can be used within the function definition - in this case, get an ID for a bitmap entry.

I know C/C++ a decade ago, bolstered thanks to my college studies, which is starting to pay off.
Title: Re: Let's talk about bmpman
Post by: The E on December 17, 2015, 05:02:00 am
Quote from: Bryan See on December 16, 2015, 11:27:27 pm
The static keyword is used to declare variables and functions at global scope, namespace scope, class scope and local scope. Static objects or variables are allocated when the program starts and is deallocated when the program ends. In this case here, an ID is initialized upon starting FSO, and it deallocated when FSO closes.

See, I can kinda see why you did this the way you did, but it doesn't look like a well thought-out design to me. You are using a design pattern that is used nowhere else in FSO, and one that is deeply unintuitive to boot (If only because of naming issues; "get_id" does not sound like the kind of function that should have global side effects the way it does in your snippet there).
Secondly, I think you're mistaking the high-level intent ("Bitmap ids should be unique and valid throughout the lifetime of the program") for a low-level implementation detail.

Quote
Templates are the foundation of generic programming, which involves writing code in a way that is independent of any particular type. Here in my code snippet, type is a placeholder name for a data type used by the function. This name can be used within the function definition - in this case, get an ID for a bitmap entry.

Why do you believe that templates are useful here? There is nothing in the bm_entry struct that needs to have templating applied to it.

Again, your understanding of C/C++, the engine, and now apparently software design principles, is not at the level required to do this sort of work. If you want to help out, you should pick an easier project first, because right now, we'd have to hold your hand through the entire process. We have neither the time nor the inclination to do so.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 17, 2015, 05:47:12 am
What kind of easy project within SCP I can pick, The E? I wanted to help out the SCP, after all of these false starts happened back in 2009 prior to my college days... I think it's simply due to my current programming level, which is below the level needed to do this sort of work. I'm making my Shattered Stars campaign/mod. This thread is nothing but a discussion about what to do with BMPMAN, which you said is almost hitting its limits.
Title: Re: Let's talk about bmpman
Post by: Goober5000 on December 19, 2015, 05:06:39 pm
A great way to learn the code (albeit with a steep learning curve) is by tracing through it to track down bugs.  There are plenty of bugs in Mantis, and some are simple enough that a person with little code expertise could squash them in a couple hours.

Here (http://scp.indiegames.us/mantis/view.php?id=2892) is one with a pretty straightforward fix.  There are others as well.
Title: Re: Let's talk about bmpman
Post by: Bryan See on December 19, 2015, 10:59:40 pm
Thanks. I'll look into it.

Now then, back to the discussion on how BMPMAN should be changed according to the times where highly modern computers are in use.
Title: Re: Let's talk about bmpman
Post by: z64555 on December 20, 2015, 02:33:56 am
 :blah: um. ok.

Anyway. Just a heads up, I should be "useful" starting near the end of this month or maybe by mid-January. Finally got some life stuffs out of the way.

Something that would really help BMPMAN in terms of maintainability is the usage of some standard containers such as the dense_hash_map by boost (I think). If someone can get a logic flow chart of BMPMAN in action as well as some use cases in graphic form, that should help the rest of the team get a general idea of what's going on.

AFAIK, the actual image data for each texture/map is stored on the heap, while the management structure is stored on a proprietary container.

I also read somewhere that the old MacOS memory system used a lookup table and handle system, which sounds awfully like BMPMAN. Could be worth investigation.
Title: Re: Let's talk about bmpman
Post by: niffiwan on December 20, 2015, 05:38:10 am
well, since I've been spelunking in bmpman for adding apngs, have my brain dump on the use cases for animations. Please post any corrections or additions required!

General
The "management structure" is an array of bitmap_entry structs
The following functions are in loose order of how a bitmap slot travels through its lifecycle
  bm_load: creates & populates a new slot; does not load image data
  bm_load_animation: like bm_load except for animations
  bm_create: creates & populates a new slot; slot is created as a USER slot which means data *is* loaded when this function runs and bmpman won't allocate/load it
  bm_lock: load image data for a slot (heap allocated); knows how to handle every type of image file supported by FSO; can also convert image data between different bit depths
  bm_unlock: reduces the reference count on a slot (nothing else!)
  bm_unload: free's the image data but leaves the rest of the slot alone; a slot must be unlocked to be unloaded
  bm_release: free's the image data & resets the slot so it can be reused; ignores slots that are locked or loaded

Streaming animations
ani's (and apngs when done) use a single bmpman slot created by bm_create (i.e. type USER so allocate that data yourself); each frame has its data loaded into the same slot when the frame is displayed
eff's use two slots created by bm_load; 1st slot gets the 1st frames data (I think?), 2nd slot gets all the other frames data loaded in as required

Non-streaming animations
All frames are loaded into sequential/contiguous slots; the slots are allocated by bm_load_animation; much of the code that accesses animations assumes that frame X is at the slot of frame 0 + X
bm_lock/bm_unload/bm_release affect all frames in an animation (bm_unlock doesn't though?)