Add options to split framebuffers/z-buffer and better alloc assert option#150
Add options to split framebuffers/z-buffer and better alloc assert option#150Aegiker wants to merge 3 commits intoHackerN64:develop/2.1.0from
Conversation
… alloc failure asserts
Yanis002
left a comment
Yanis002
left a comment
There was a problem hiding this comment.
sorry for the late review, looks good to me but I'll wait few days just in case someone with more knowledge than me wants to comment this
works on my machine™
|
merging this tomorrow unless there's last-minute comments |
Thar0
left a comment
Thar0
left a comment
There was a problem hiding this comment.
I think the address assignments for at least the z-buffer should be moved to the spec. That way you don't need to use a macro like GET_ZBUFFER and can continue to use gZBuffer everywhere, which is a much less intrusive change.
include/macros.h
Outdated
There was a problem hiding this comment.
I don't think we should be returning a pointer to the z-buffer in a function that returns framebuffer pointers
I think this should be moved to the spec and do all the necessary address assignments there:
beginseg
name "buffers"
flags NOLOAD
include "$(BUILD_DIR)/src/buffers/gfxbuffers.o"
include "$(BUILD_DIR)/src/buffers/audio_heap.o"
#if !ENABLE_SPLIT_ZBUFFER
include "$(BUILD_DIR)/src/buffers/zbuffer.o"
#endif
endseg
#if ENABLE_SPLIT_ZBUFFER
beginseg
name "zbuffer"
flags NOLOAD
#if ENABLE_SPLIT_FRAMEBUFFERS
address 0x80500000 /* Leave room for the split framebuffers in 2 banks */
#else
address 0x80600000 /* Both framebuffers in 1 bank */
#endif
include "$(BUILD_DIR)/src/buffers/zbuffer.o"
endseg
#endif
src/code/sys_cfb.c
Outdated
There was a problem hiding this comment.
This is going to break without warning if anyone tries to use it with only 4MB
There was a problem hiding this comment.
looking at this we actually kinda worked for 2 months getting 4mb mode working and im unsure if it still works
|
ok, made the changes. if someone who can build this more quickly wouldn't mind stress testing it I would appreciate that |
|
I get the feeling we're still missing lots of cases here, e.g. it looks like 4MB mode is still going to break without warning. I thought about all the possible cases that would be useful: In 4MB mode we don't have enough memory to do particularly meaningful splits, we can't split the z-buffer and both framebuffers with only 4MB since it would complicate the memory map too much, and we can't reserve entire banks for anything. In 8MB mode we have a lot more flexibility. We can choose to burn lots of memory to achieve the maximum possible RDP buffer performance, or we can be a little more conservative of the memory. This should be up to the user to configure based on their needs: the option I called "ramsave" below will reclaim memory at the cost of performance. idk if we want to be this flexible, but if the goal is to be a useful base for lots of different uses it seems advantageous. |
|
Interesting, though I think it's already flexible enough as is. You can already split the Z-buffer without splitting the framebuffer, which I think is generous. I disagree that 4MB would crash without warning, because I wrote a warning above the option and tried to make it clear what would happen if you did. If you're using 4MB you probably have bigger problems anyway, and it's not an argument to have here but I don't think extended support for 4MB is a good idea in general. If you wanted to implement this it certainly wouldn't hurt, but even though it's possible I don't think the proposal is terribly useful, and I wouldn't have any interest in implementing this. I don't think that should prevent the sufficient functionality here from being merged though, because it works as intended. |
imo it's not enough to just say so in a comment. We can do better and make the build fail with an error message.
Personally I have no interest in 4MB, I'd never use it. But if the goal is to reach lots of users it's not really ok to just ignore 4MB, unless we specifically define what our boundaries are with respect to supporting it. If we disagree on what these boundaries are then that probably needs to be worked out to decide how to proceed |
|
I don't know if there is an established method for making the build fail with an error message, that'd be fine. 4MB support is... fine, but I think that extended support for things like this is an unnecessary hindrance. Options are cool, and you know I try to defend the underdog when the discussion is about console compatibility. Still, too many options creates bloat, and not everything that could be one should be one. 4MB users have the option to not use this, so it does not take away the choice to use 4MB. 8MB exists for a reason, and should be allowed to be used to its fullest potential. 4MB is not the best option with regards to performance and I don't think I should have to go out of my way to support extra diminished versions of this just because they could exist, especially if it denies users access to what is already available because it got stuck in tape. This applies to everything. If full support for both is the standard, the debug heap should be fixed too instead of overriding SYS_CFB_END, but it's not realistic to expect everything to work and I don't think that anyone does. |
essentially the main requirement camera debug was completely disabled. the rest wasnt a hard requirement. i think that this define should just denable 4mb mode entirely. as what your trying to do kills it anyway. thats the simplest solution and the easiest. |
|
let me know when this is ready to merge |
|
(same as my last comment to get this merged) |
|
Well there are a couple of merge conflicts now but the behavior is as intended so imo it can be merged |
4 participants
No description provided.