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.

Sign up for GitHub

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

Bugfix/merged optimizations #578

Closed
wants to merge 4 commits into from
+63 −6
Closed

Bugfix/merged optimizations #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
737df0a
Ensure single buffers for OpenGL objects, execute pending mask update…
maurhofer-ubique Jan 8, 2024
c65df0f
ignore animations that are only cached symbols
maerki Jan 13, 2024
2074653
Call glFinish on render breaks
maurhofer-ubique Jan 15, 2024
50da5a7
Debounce new tile loading, use actor mutex for thread-safety
maurhofer-ubique Jan 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions android/src/main/cpp/graphics/objects/Quad2dInstancedOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ void Quad2dInstancedOpenGl::prepareGlData(int program) {
glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(GLubyte) * indices.size(), &indices[0], GL_STATIC_DRAW);
glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);

if (!glDataBuffersGenerated) {
glGenBuffers(1, &positionsBuffer);
glGenBuffers(1, &rotationsBuffer);
glGenBuffers(1, &textureCoordinatesListBuffer);
glGenBuffers(1, &scalesBuffer);
glGenBuffers(1, &alphasBuffer);
}
instPositionsHandle = glGetAttribLocation(program, "aPosition");
instRotationsHandle = glGetAttribLocation(program, "aRotation");
instTextureCoordinatesHandle = glGetAttribLocation(program, "aTexCoordinate");
Expand Down
8 changes: 8 additions & 0 deletions android/src/main/cpp/graphics/objects/Quad2dStretchedInstancedOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,14 @@ void Quad2dStretchedInstancedOpenGl::prepareGlData(int program) {
glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(GLubyte) * indices.size(), &indices[0], GL_STATIC_DRAW);
glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);

if (!glDataBuffersGenerated) {
glGenBuffers(1, &positionsBuffer);
glGenBuffers(1, &textureCoordinatesListBuffer);
glGenBuffers(1, &scalesBuffer);
glGenBuffers(1, &rotationsBuffer);
glGenBuffers(1, &alphasBuffer);
glGenBuffers(1, &stretchInfoBuffer);
Comment on lines +106 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

The conditional block for generating OpenGL buffers only if they haven't been generated before is a good practice to avoid redundant operations. However, there is no corresponding flag set to true for glDataBuffersGenerated after the buffers are generated. This could lead to the buffers not being generated at all if glDataBuffersGenerated is not properly initialized to false.

if (!glDataBuffersGenerated) {
    glGenBuffers(1, &positionsBuffer);
    glGenBuffers(1, &textureCoordinatesListBuffer);
    glGenBuffers(1, &scalesBuffer);
    glGenBuffers(1, &rotationsBuffer);
    glGenBuffers(1, &alphasBuffer);
    glGenBuffers(1, &stretchInfoBuffer);
    glDataBuffersGenerated = true; // This line is missing to indicate that the buffers have been generated.
}

}
instPositionsHandle = glGetAttribLocation(program, "aPosition");
instTextureCoordinatesHandle = glGetAttribLocation(program, "aTexCoordinate");
instScalesHandle = glGetAttribLocation(program, "aScale");
Expand Down
9 changes: 9 additions & 0 deletions android/src/main/cpp/graphics/objects/Text2dInstancedOpenGl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,15 @@ void Text2dInstancedOpenGl::prepareGlData(int program) {
glBufferData(GL_ELEMENT_ARRAY_BUFFER, sizeof(GLubyte) * indices.size(), &indices[0], GL_STATIC_DRAW);
glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);

if (!glDataBuffersGenerated) {
glGenBuffers(1, &positionsBuffer);
glGenBuffers(1, &textureCoordinatesListBuffer);
glGenBuffers(1, &scalesBuffer);
glGenBuffers(1, &rotationsBuffer);
glGenBuffers(1, &styleIndicesBuffer);
glGenBuffers(1, &styleBuffer);
}

instPositionsHandle = glGetAttribLocation(program, "aPosition");
instTextureCoordinatesHandle = glGetAttribLocation(program, "aTexCoordinate");
instScalesHandle = glGetAttribLocation(program, "aScale");
Expand Down
1 change: 1 addition & 0 deletions android/src/main/java/io/openmobilemaps/mapscore/graphics/GLThread.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ class GLThread constructor(
hasFinishedSinceDirty = true
finishDuration = System.currentTimeMillis() - finishDuration
}
val finishDuration = System.currentTimeMillis() - preFinish
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable finishDuration is redeclared, which will cause a compilation error in Kotlin.

- val finishDuration = System.currentTimeMillis() - preFinish

Remove the redeclaration of finishDuration to use the previously declared variable.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
val finishDuration = System.currentTimeMillis() - preFinish


try {
if (finishDuration < BREAK_RENDER_INTERVAL) {
Expand Down
6 changes: 5 additions & 1 deletion shared/public/Tiled2dMapSource.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,9 @@ class Tiled2dMapSource :
void onVisibleTilesChanged(const std::vector<VisibleTilesLayer> &pyramid, int keepZoomLevelOffset = 0);

private:
void performLoadingTask(Tiled2dMapTileInfo tile, size_t loaderIndex);
void onStableTriggerNewTileLoading(uint64_t updateId);
Copy link
Contributor

Choose a reason for hiding this comment

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

The method onStableTriggerNewTileLoading has been added but lacks documentation.

Please add documentation to explain the purpose and usage of this method.


void performLoadingTask(Tiled2dMapTileInfo tile, size_t loaderIndex);

void updateTileMasks();

Expand All @@ -181,11 +182,14 @@ class Tiled2dMapSource :

std::unordered_map<size_t, std::map<Tiled2dMapTileInfo, ErrorInfo>> errorTiles;
std::optional<long long> nextDelayTaskExecution;
std::vector<PrioritizedTiled2dMapTileInfo> pendingTilesToLoad;
Copy link
Contributor

Choose a reason for hiding this comment

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

The member pendingTilesToLoad has been added but lacks documentation.

Please add documentation to explain the purpose and usage of this member.


std::unordered_set<Tiled2dMapTileInfo> notFoundTiles;

std::string layerName;

uint64_t currentNewTilesUpdateId = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

The member currentNewTilesUpdateId has been added but lacks documentation.

Please add documentation to explain the purpose and usage of this member.

const static int64_t PENDING_NEW_TILE_DEBOUNCE_MS = 200;
Copy link
Contributor

Choose a reason for hiding this comment

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

The constant PENDING_NEW_TILE_DEBOUNCE_MS has been added but lacks documentation.

Please add documentation to explain the purpose and usage of this constant.

};

#include "Tiled2dMapSourceImpl.h"
33 changes: 29 additions & 4 deletions shared/public/Tiled2dMapSourceImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,17 +361,42 @@ void Tiled2dMapSource<T, L, R>::onVisibleTilesChanged(const std::vector<VisibleT
}
}

std::sort(toAdd.begin(), toAdd.end());

for (const auto &addedTile : toAdd) {
performLoadingTask(addedTile.tileInfo, 0);
if (auto strongScheduler = scheduler.lock()) {
std::lock_guard<std::recursive_mutex> lock(mailbox->receivingMutex);
std::weak_ptr<Tiled2dMapSource> weakSelfPtr = std::dynamic_pointer_cast<Tiled2dMapSource>(
shared_from_this());
std::sort(toAdd.begin(), toAdd.end());
if (toAdd != pendingTilesToLoad) {
long newId = ++currentNewTilesUpdateId;
pendingTilesToLoad = toAdd;
strongScheduler->addTask(std::make_shared<LambdaTask>(
TaskConfig("DebounceNewTilesLoad_" + std::to_string(newId), PENDING_NEW_TILE_DEBOUNCE_MS, TaskPriority::NORMAL,
ExecutionEnvironment::COMPUTATION),
[weakSelfPtr, newId] {
if (auto strongSelf = weakSelfPtr.lock()) {
strongSelf->onStableTriggerNewTileLoading(newId);
}
}));
}
}

//if we removed tiles, we potentially need to update the tilemasks - also if no new tile is loaded
updateTileMasks();

notifyTilesUpdates();
}

template<class T, class L, class R>
void Tiled2dMapSource<T, L, R>::onStableTriggerNewTileLoading(uint64_t updateId) {
for (const auto &tileToLoad : pendingTilesToLoad) {
std::lock_guard<std::recursive_mutex> lock(mailbox->receivingMutex);
if (updateId != currentNewTilesUpdateId) {
break;
}
performLoadingTask(tileToLoad.tileInfo, 0);
}
}

template<class T, class L, class R>
void Tiled2dMapSource<T, L, R>::performLoadingTask(Tiled2dMapTileInfo tile, size_t loaderIndex) {
if (currentlyLoading.count(tile) != 0) return;
Expand Down
3 changes: 2 additions & 1 deletion shared/public/Value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,8 @@ class ValueEvaluator {
}

if (isZoomDependent || (isStateDependant && !context.featureStateManager->empty())) {
return value->evaluateOr(context, defaultValue);
auto result = value->evaluateOr(context, defaultValue);
return result;
Comment on lines +657 to +658
Copy link
Contributor

Choose a reason for hiding this comment

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

The ValueEvaluator class has been modified to assign the result of value->evaluateOr(context, defaultValue) to a local variable result before returning it. This change is redundant as the result is not used for any processing and is immediately returned. This adds unnecessary lines of code and may slightly impact performance due to the additional variable assignment.

- auto result = value->evaluateOr(context, defaultValue);
- return result;
+ return value->evaluateOr(context, defaultValue);
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
auto result = value->evaluateOr(context, defaultValue);
return result;
return value->evaluateOr(context, defaultValue);

}

auto identifier = context.feature->identifier;
Expand Down
2 changes: 2 additions & 0 deletions shared/src/map/layers/tiled/vector/symbol/Tiled2dMapVectorSymbolObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class Tiled2dMapVectorSymbolObject {

void removeFromCache();

void removeFromCache();

Comment on lines +61 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate removeFromCache method definition.

- void removeFromCache();

Remove the duplicate method definition to prevent compilation errors.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
void removeFromCache();

struct SymbolObjectInstanceCounts { int icons, textCharacters, stretchedIcons; };

const SymbolObjectInstanceCounts getInstanceCounts() const;
Expand Down
Loading