Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add transparent index and optimize font rendering with shadows. #730

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nopjne
Copy link

@nopjne nopjne commented Nov 2, 2024

Reduces texture switches when rendering fonts with shadows.
Introduces transparent index as an internal value supported by the entre sprite implementation.
Also puts all different transparent indices in one spot.

src/formats/palette.h Outdated Show resolved Hide resolved
src/formats/sprite.c Outdated Show resolved Hide resolved
src/formats/vga_image.c Show resolved Hide resolved
src/game/gui/gauge.c Outdated Show resolved Hide resolved
src/formats/palette.h Outdated Show resolved Hide resolved
src/video/surface.c Outdated Show resolved Hide resolved
src/formats/palette.h Outdated Show resolved Hide resolved
@Vagabond
Copy link
Member

Vagabond commented Nov 3, 2024

Merging #732 broke this PR a bit. I wonder if you'd be open, @nopjne to me working on some code to separate the text flowing from the text rendering. So flowing some text would result in an array of surface pointers and the offsets to render them at. Then you could do a 2 pass render to render the shadows first and get the same optimization as you had here, but with the benefit of not having to reflow text each frame?

@nopjne
Copy link
Author

nopjne commented Nov 3, 2024

Merging #732 broke this PR a bit. I wonder if you'd be open, @nopjne to me working on some code to separate the text flowing from the text rendering. So flowing some text would result in an array of surface pointers and the offsets to render them at. Then you could do a 2 pass render to render the shadows first and get the same optimization as you had here, but with the benefit of not having to reflow text each frame?

I was sort of hoping 732 would be merged after this PR was done, but it is what it is.

I also have a change that buffers the flowing of text, please take a look at the text_object in the n64 if you plan on working on this.
It's a big change I don't really want to do much merging on. Most important parts are ability to prerender letters to a texture, and allow a way to invalidate the cache. Also my change allows declaring the text_object as frame_over_frame copy safe, and I make heavy use of it in the N64 branch where I copy the rectangle from the previous frame, without having to allocate memory for a text_object.

@nopjne nopjne force-pushed the master branch 11 times, most recently from 6c3261e to 4bcaa89 Compare November 7, 2024 05:42
@nopjne nopjne force-pushed the master branch 12 times, most recently from 780bf67 to f5fe82c Compare November 17, 2024 13:38
@nopjne nopjne force-pushed the master branch 2 times, most recently from 6aa0f28 to 7ebfa7b Compare November 17, 2024 14:01
@nopjne nopjne requested a review from katajakasa November 17, 2024 18:30
src/formats/transparent.h Outdated Show resolved Hide resolved
src/game/scenes/melee.c Outdated Show resolved Hide resolved
@nopjne nopjne force-pushed the master branch 2 times, most recently from 975b819 to 9d27488 Compare November 17, 2024 22:28
@@ -35,4 +39,16 @@ static inline void *omf_realloc_real(void *ptr, size_t size, const char *file, i
abort();
}

static inline void *omf_alloc_with_options_real(size_t nmemb, size_t size, int options, const char *file, int line) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the options ? Could this just be omF_alloc_surface_data() or somesuch ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could, though keeping it more abstract allows us to reduce the number of functions in the API and changes don't need to touch the common code if adding new options. It also allows implementations to ignore new options when they are added, instead of having to implement the missing functions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have three issues with that:

  1. We don't currently know of any cases that need more flags, making this an unnecessary complication
  2. Free function also requires the same flags as the alloc, which us ugly
  3. Why do we need a bitmask ? malloc cases tend to be pretty specific anyways, making separate functions clearer in the long run.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We don't currently know of any cases that need more flags, making this an unnecessary complication

Any allocation that could be used by hardware directly, will need to go through alloc with options.

  1. Free function also requires the same flags as the alloc, which us ugly

This is more of a stylistic preference why is a set of flags and 1 free-function uglier than a set of free-functions names.

  1. Why do we need a bitmask ? malloc cases tend to be pretty specific anyways, making separate functions clearer in the long run.

Allocation parameters may be different, cached/uncached, aligned/unaligned, size-extended/non-extended, different alginment requirements for different allocation types.
My gripe with separate functions is that you will have to rev all your platforms every time a new function is added and most platforms won't do anything special or different. So just increasing the merge conflict size, when multiple people are working on that code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bitflags are unsifficient for arbitrarily aligned allocations and allocate_at_least style allocations.

Arbitrarily (un)aligned allocations do not map particularly well to bitflags, as the caller needs to control to what the allocation is aligned-- 32-byte alignment for AVX, 16-byte for SSE, etc.. I'd expect any aligned allocator we provide to be similar to aligned_alloc.

size-extended allocations.. sounds like when vector.h asks for an allocation of size 12 (for 3 4-byte elements), and the allocator decides to round up to some common size like 16-- the allocator must have a way of passing the size of the allocation to an interested caller (see C++'s allocate_at_least), preferrably without the overhead of storing alloc size in every allocation (as glibc does for malloc_usable_size).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bitflags are unsifficient for arbitrarily aligned allocations and allocate_at_least style allocations.

I've thought about this too and initially thought it wouldn't be sufficient but warmed up to the idea of letting the platform decide on the allocation type based on the incoming flags.
The flags are very high level but allow the platform to make a decision if it should be any of the above types. While alloc_aligned may make sense on one platform it doesn't have to make sense on another and can be unaligned somewhere else.

src/utils/allocator_n64.h Outdated Show resolved Hide resolved
Comment on lines +41 to +42
#LibPNG is not linking on MSVC correctly, disable it.
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -DMIN_BUILD")
Copy link
Contributor

@Nopey Nopey Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not. see also #769 (merged)

Copy link
Author

@nopjne nopjne Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was temporary you can take it out when 769 is merged. Or I'll discard it it if 769 goes in first.

//
// NOTE: An allocation with a specific set of options also requires a matching free with the same set of options.
//
#define omf_alloc_with_options(nmemb, size, options) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the options is going to declare different types of allocations, then what happens if ALLOC_HINT_TEXTURE and e.g. ALLOC_HINT_SOUND are defined simultaneously ? This is a very dangerous thing for undefined behavirour. Perhaps the flags should be an enum instead.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a platform can't support an allocation that's both used as a texture and a sound buffer, it will need to fall back to the common denominator, which will be allocating the memory as a regular malloc here and creating a copy in the hardware implementations for this buffer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth thinking about what allocator flags we'd actually want to support. Obviously some combinations are nonsensical (like TEXTURE | SOUND) but we could also add a simple check, in debug mode, for invalid flag combinations, if we wished.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Srsly, it might be best to split this to functions by purpose, and then allow more detailed flags for those as required. So malloc_texture(amount, flags), malloc_sound(amount, flags) etc. That would prevent nonsensical combinations, and just reserve the options flag for fine-grained changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other use, other than n64 support, that I can think of would be an arena style allocator, where we could indicate whether an allocation should live for the life of the scene (in the scene arena allocation), in a global/engine arena or should be manually managed (a more ephemeral object). Then we could use the hints to track the kind of allocation we're doing.

Copy link
Member

@katajakasa katajakasa Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to use some arena allocator also. If someone has the interest, I would be all for seeing one integrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants