[Bryan See]

And I started doing it.

First of all, I've cleaned up all of the unused ones, and add a barebones ID code to identify the bitmap IDs.

[The E]

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?

[AdmiralRalwood]

If GitHub won't show the diff of bmpman.cpp due to being too long, I've pastebinned a diff of the commit here.

[The E]

Yeah, that's definitely an earlier version of that file.

[Bryan See]

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.

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.

[The E]

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.

[Bryan See]

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.

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?

[The E]

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.

[Bryan See]

OK, noted. I'll start doing it right away.

[AdmiralRalwood]

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.

[Bryan See]

Okay, here's the commit 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.

[AdmiralRalwood]

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.

[Bryan See]

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.

[AdmiralRalwood]

I've squashed these two commits already on GitHub.

No you haven't; they're clearly still two separate commits.

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.

[Bryan See]

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.

[The E]

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.

[Bryan See]

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.

[The E]

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?

[Bryan See]

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.

[The E]

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.

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.

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.