OpenGL Labels #72

LucasDworschak · 2024-01-03T15:58:00Z

No description provided.

- added quads on future label positions that stay the same size regardless of camera position and always face the camera

- Labels are now fully done in the shader

…ls into labels

- added quads on future label positions that stay the same size regardless of camera position and always face the camera

- Labels are now fully done in the shader

- separated code into nucleus / gl_engine - only one draw call for all labels and implemented instancing

- svg icon below the label

- unicode char in string to integer list (correct unicode decoding) - only render characters that are needed (until now we rendered chars 32-223 on the ascii table, now we can specify each char individually) - characters now contain an outline

- minor code structure improvements - add uniform shader variable option for distance scaling

- draw "space" character if char is not available in the font_atlas

- occlusion culling - distance fadeout - code structure refactoring

- improved label fadeout

adam-ce

very nice changes. i'm super happy that we have much improved labels now.

in general i like the code, there are some minor code style issues detailed in the comments.

I'll push some changes to radix very soon, because currently address sanitizer is broken. they'll also enable warnings (just fix your own).

Workflow:
update your labels branch, the changes will be automatically forwarded to this pull request. this also means, that you should not mix in the changes from the vector labels (create an extra branch). then click on request review in this pull request.

In addition, AddressSanitizer shows me the following issue (i don't think it's the possible leak i mentioned below):

=================================================================
==64331==ERROR: AddressSanitizer: heap-use-after-free on address 0x7f15601c4278 at pc 0x55fa84a63e2e bp 0x7f1553d012b0 sp 0x7f1553d012a0
READ of size 1 at 0x7f15601c4278 thread T4 (QSGRenderThread)
    #0 0x55fa84a63e2d in ttSHORT /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/extern/stb_slim/src/stb_slim/stb_truetype.h:1287
    #1 0x55fa84a63e2d in stbtt_ScaleForPixelHeight /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/extern/stb_slim/src/stb_slim/stb_truetype.h:2662
    #2 0x55fa84a4dec6 in nucleus::MapLabel::create_text_meta(std::unordered_map<int, nucleus::MapLabel::CharData const, std::hash<int>, std::equal_to<int>, std::allocator<std::pair<int const, nucleus::MapLabel::CharData const> > > const&, stbtt_fontinfo const*, std::vector<int, std::allocator<int> >&, float&) /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/nucleus/map_label/MapLabel.cpp:75
    #3 0x55fa84a4dec6 in nucleus::MapLabel::init(std::unordered_map<int, nucleus::MapLabel::CharData const, std::hash<int>, std::equal_to<int>, std::allocator<std::pair<int const, nucleus::MapLabel::CharData const> > > const&, stbtt_fontinfo const*) /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/nucleus/map_label/MapLabel.cpp:45
    #4 0x55fa84a7ea8a in nucleus::MapLabelManager::init() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/nucleus/map_label/MapLabelManager.cpp:94
    #5 0x55fa84a412db in gl_engine::MapLabelManager::init() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/gl_engine/MapLabelManager.cpp:32
    #6 0x55fa849f4015 in gl_engine::Window::initialise_gpu() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/gl_engine/Window.cpp:147
    #7 0x55fa847f3872 in TerrainRenderer::TerrainRenderer() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/app/TerrainRenderer.cpp:37
    #8 0x55fa847dfbd5 in TerrainRendererItem::createRenderer() const /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/app/TerrainRendererItem.cpp:101
    #9 0x7f1573d6f7d0 in QQuickFramebufferObject::updatePaintNode(QSGNode*, QQuickItem::UpdatePaintNodeData*) (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x4337d0)
    #10 0x7f1573bba81f in QQuickWindowPrivate::updateDirtyNode(QQuickItem*) (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x27e81f)
    #11 0x7f1573bbb13b in QQuickWindowPrivate::updateDirtyNodes() (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x27f13b)
    #12 0x7f1573bbeab3 in QQuickWindowPrivate::syncSceneGraph() (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x282ab3)
    #13 0x7f1573d7723f  (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x43b23f)
    #14 0x7f1573d78663  (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x43c663)
    #15 0x7f1573d795c6  (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x43d5c6)
    #16 0x7f15718619dc  (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Core.so.6+0x2f29dc)
    #17 0x7f15710a8ac2 in start_thread nptl/pthread_create.c:442
    #18 0x7f157113a65f  (/lib/x86_64-linux-gnu/libc.so.6+0x12665f)

0x7f15601c4278 is located 367224 bytes inside of 524288-byte region [0x7f156016a800,0x7f15601ea800)
freed by thread T4 (QSGRenderThread) here:
    #0 0x7f1574105537 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127
    #1 0x55fa84a7df46 in QArrayDataPointer<char>::~QArrayDataPointer() /home/madam/bin/Qt/6.6.1/gcc_64/include/QtCore/qarraydatapointer.h:104
    #2 0x55fa84a7df46 in QByteArray::~QByteArray() /home/madam/bin/Qt/6.6.1/gcc_64/include/QtCore/qbytearray.h:521
    #3 0x55fa84a7df46 in nucleus::MapLabelManager::create_font() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/nucleus/map_label/MapLabelManager.cpp:202
    #4 0x55fa84a7e964 in nucleus::MapLabelManager::init() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/nucleus/map_label/MapLabelManager.cpp:91
    #5 0x55fa84a412db in gl_engine::MapLabelManager::init() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/gl_engine/MapLabelManager.cpp:32
    #6 0x55fa849f4015 in gl_engine::Window::initialise_gpu() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/gl_engine/Window.cpp:147
    #7 0x55fa847f3872 in TerrainRenderer::TerrainRenderer() /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/app/TerrainRenderer.cpp:37
    #8 0x55fa847dfbd5 in TerrainRendererItem::createRenderer() const /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/app/TerrainRendererItem.cpp:101
    #9 0x7f1573d6f7d0 in QQuickFramebufferObject::updatePaintNode(QSGNode*, QQuickItem::UpdatePaintNodeData*) (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Quick.so.6+0x4337d0)

previously allocated by thread T4 (QSGRenderThread) here:
    #0 0x7f1574105887 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x7f15717e1d34 in QArrayData::allocate(QArrayData**, long long, long long, long long, QArrayData::AllocationOption) (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Core.so.6+0x272d34)

Thread T4 (QSGRenderThread) created by T0 here:
    #0 0x7f15740a9685 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:216
    #1 0x7f157186144c in QThread::start(QThread::Priority) (/home/madam/bin/Qt/6.6.1/gcc_64/lib/libQt6Core.so.6+0x2f244c)

SUMMARY: AddressSanitizer: heap-use-after-free /home/madam/Documents/work/tuw/alpinemaps/renderer_labels/extern/stb_slim/src/stb_slim/stb_truetype.h:1287 in ttSHORT
Shadow bytes around the buggy address:
  0x0fe32c0307f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030800: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030810: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030820: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030830: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0fe32c030840: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]
  0x0fe32c030850: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030860: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030870: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030880: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0fe32c030890: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==64331==ABORTING

adam-ce · 2024-01-09T13:47:49Z

nucleus/map_label/MapLabel.h


-namespace nucleus::map_label {
+#include "stb_slim/stb_truetype.h"


replace with struct stbtt_fontinfo; and move the import to the cpp file

adam-ce · 2024-01-09T13:51:18Z

nucleus/map_label/MapLabelManager.cpp

+{
+    // create the final font bitmap (3 channels -> 1 for font; 1 for outline; the last channel is empty)
+    const int channels = 3;
+    m_font_bitmap = new uint8_t[m_font_atlas_size.width() * m_font_atlas_size.height() * channels];


please no raw new and delete. these are very C like. use RAII, which is how you should do things in C++ :)

a good replacement would be std::vector<uint8_t> + resize()

or, in this case, a nucleus::Raster<uint8_t> (you would have to add a resize and fill method)

adam-ce · 2024-01-09T13:54:59Z

nucleus/map_label/MapLabelManager.cpp

+    const int channels = 3;
+    m_font_bitmap = new uint8_t[m_font_atlas_size.width() * m_font_atlas_size.height() * channels];
+    // set everything to 0
+    memset(m_font_bitmap, 0, m_font_atlas_size.width() * m_font_atlas_size.height() * channels);


memset is very c-like. please use c++. use e.g. std::fill

adam-ce · 2024-01-09T13:56:27Z

nucleus/map_label/MapLabelManager.cpp

+    memset(m_font_bitmap, 0, m_font_atlas_size.width() * m_font_atlas_size.height() * channels);
+
+    const int outline_bit_count = 4 * (m_font_outline.x - 1) * (m_font_outline.y - 1) + 2 * (m_font_outline.x - 1) + 2 * (m_font_outline.y - 1) + 1;
+    int* outline_mask = new int[outline_bit_count];


same here. please use std::vector

adam-ce · 2024-01-09T13:57:45Z

nucleus/map_label/MapLabelManager.cpp

+    const auto font_init = stbtt_InitFont(&m_fontinfo, reinterpret_cast<const uint8_t*>(data.constData()), stbtt_GetFontOffsetForIndex(reinterpret_cast<const uint8_t*>(data.constData()), 0));
+    assert(font_init);
+
+    uint8_t* temp_bitmap = new uint8_t[m_font_atlas_size.width() * m_font_atlas_size.height()];


same here (no raw new and delete)

adam-ce · 2024-01-09T14:16:21Z

nucleus/map_label/MapLabel.cpp

+void MapLabel::init(const std::unordered_map<int, const MapLabel::CharData>& character_data, const stbtt_fontinfo* fontinfo)
+{
+    float offset_x = 0;
+    float offset_y = -font_size / 2.0f + 75.0;


this also converts float -> double -> float

adam-ce · 2024-01-09T14:16:31Z

nucleus/map_label/MapLabel.cpp

+    float offset_x = 0;
+    float offset_y = -font_size / 2.0f + 75.0;
+
+    float uv_width_norm = 1.0f / 512.0f;


adam-ce · 2024-01-09T14:21:34Z

nucleus/map_label/MapLabelManager.h

+
+private:
+    // list of all characters that will be available (will be rendered to the font_atlas)
+    const std::string all_char_list = " ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789()[]{},;.:-_!\"§$%&/\\=+-*/#'~°^<>|@€´`öÖüÜäÄß";


this could be a static constexpr auto all_char_list

adam-ce · 2024-01-09T14:27:13Z

nucleus/map_label/MapLabelManager.h

+    std::unordered_map<int, const MapLabel::CharData> m_char_data;
+
+    stbtt_fontinfo m_fontinfo;
+    uint8_t* m_font_bitmap;


no naked new and delete. i believe this one we are even leaking :)

you can use the address sanitizer to detect such issues (enable the cmake variable ALP_ENABLE_ADDRESS_SANITIZER, but you have to pull first)

adam-ce · 2024-01-09T14:32:03Z

gl_engine/shaders/labels.vert

+     // we are interpolating the far label according to importance
+    float interpolatedFarLabelDistance = mix(farLabel0, farLabel1, importance);
+
+    // apply "soft" distance scaling depending on near/far label values (if option is set as uniform)


the following gives me nicer results:
` // apply "soft" distance scaling depending on near/far label values (if option is set as uniform)
if(label_dist_scaling)
{
float dist_scale = 1.0 - ((dist - nearLabel) / (farLabel1 - nearLabel)) * 0.4f;
scale *= (dist_scale * dist_scale);
}

// importance based scaling scale *= (importance + 1.5) / 2.5;

`
and with label_dist_scaling set to true

LucasDworschak · 2024-01-17T15:11:53Z

latest commit should address all the requested changes.

LucasDworschak · 2024-01-17T18:53:50Z

closing pull request and reopening from main (as discussed in discord)

LucasDworschak added 20 commits November 28, 2023 13:33

Labels as a Shader

5678ad9

- added quads on future label positions that stay the same size regardless of camera position and always face the camera

Labels as a Shader

685e589

- Labels are now fully done in the shader

Merge branch 'labels' of github.com:LucasDworschak/AlpineMapsOrg_Labe…

ce7583a

…ls into labels

moved stb_truetype.h to external library

ca599bc

Labels as a Shader

38fa37f

- added quads on future label positions that stay the same size regardless of camera position and always face the camera

Labels as a Shader

5dec03d

- Labels are now fully done in the shader

moved stb_truetype.h to external library

ab52022

Updated changes from main

97c7478

Moved label scaling calculation into shader

dfd2fec

Map Label:

5917bf7

- separated code into nucleus / gl_engine - only one draw call for all labels and implemented instancing

applied correct kerning to the labels

c87aecc

Map Labels:

a50712c

- svg icon below the label

Map Labels:

5745588

- unicode char in string to integer list (correct unicode decoding) - only render characters that are needed (until now we rendered chars 32-223 on the ascii table, now we can specify each char individually) - characters now contain an outline

MapLabel:

0569366

- minor code structure improvements - add uniform shader variable option for distance scaling

MapLabel:

9b18fdd

- draw "space" character if char is not available in the font_atlas

MapLabels:

88d3f10

- occlusion culling - distance fadeout - code structure refactoring

Added correct stb_slim git project

842f876

added correct copyright; changed cmakelist git url to https for stb_slim

fcaf9b2

Merged with main repo

0c439f5

- Added Lucas Dworschak to about page

49ba057

- improved label fadeout

adam-ce requested changes Jan 9, 2024

View reviewed changes

LucasDworschak added 4 commits January 16, 2024 16:50

merge with upstream/main + minor bug fixes

25877a3

Bug fixes from pull request review

4bd02ab

merge with upstream main

8899da9

Bug fix: used lower case qDebug in include

20e0212

LucasDworschak added 2 commits January 17, 2024 18:40

Labels shader bugfixes for web

549d847

Labels shader additional bugfixes for web

e494ca0

LucasDworschak closed this Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenGL Labels #72

OpenGL Labels #72

LucasDworschak commented Jan 3, 2024

adam-ce left a comment •

edited

Loading

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

adam-ce Jan 9, 2024

LucasDworschak commented Jan 17, 2024

LucasDworschak commented Jan 17, 2024


		namespace nucleus::map_label {
		#include "stb_slim/stb_truetype.h"

OpenGL Labels #72

OpenGL Labels #72

Conversation

LucasDworschak commented Jan 3, 2024

adam-ce left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasDworschak commented Jan 17, 2024

LucasDworschak commented Jan 17, 2024

adam-ce left a comment •

edited

Loading